Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert to a V2 addon #1246

Open
wants to merge 91 commits into
base: master
Choose a base branch
from
Open

Convert to a V2 addon #1246

wants to merge 91 commits into from

Conversation

abeforgit
Copy link
Member

@abeforgit abeforgit commented Jan 28, 2025

Overview

Follow the commits, otherwise there's no sense in reviewing cause it basically touches all files

I initially added the new version under editor-v2, and then later moved the files back to toplevel

Breaking changes:

webpack

apps now need to import the webpack.config.js, see the ember-cli-build.js of the test-app

There's no way around this anymore as V2 addons cannot influence the build pipeline of the app. However, it's only needed for graphy, so it seems feasible to phase that out over time cause it's a bit of an annoying lib (albeit a very good one)
more info to follow

As of 4433309 (#1246) this is no longer the case! so this pr could actually be non breaking? maybe?

private things are actually private now (except the ones listed in the package.json exports field)

It is possible that I missed some private things that were imported in some of our apps. That's not technically breaking (they were private after all), but still, in practice it might break things

development changes

repo organization

As most v2 addons, this is now a monorepo. The test-app (what was previously the dummy app) lives in the test-app folder, and the editor source code lives in packages/ember-rdfa-editor. I've decided to go with this structure cause I expect to break out many of the plugins we now have in the plugins directory into their own packages.

As with all monorepos, be aware of what folder you run your pnpm commands in. pnpm will always look at the closest package.json to determine which scripts it will run.
It can be a good idea to stay in the top-level, and use the filter options pnpm provides.

UPDATE: I've added the convenience scripts "say" and "tapp" (Test-APP), which are simply pnpm --filter @lblod/ember-rdfa-editor and pnpm --filter test-app
so to add a package to the addon, you can simply do pnpm say add some-package, or to run an ember-try scenario, you can do pnpm tapp exec ember try:one embroider-optimized for example

test-app

The test-app always uses the current dev build of the editor source, because of the workspace:* specifier in its package.json for the editor package. It should also auto-reload when using the start script from the monorepo root.

imports

NOTE: this section is only relevant for the addon code, the test-app is still a normal ember-cli and broccoli app.

The idea of a v2 addon is that we write spec-compliant es modules, which then get bundled appropriately by rollup into whatever output format ember needs

ES modules look very similar to the import/export syntax you're used to, but it's not exactly the same:

see https://nodejs.org/api/esm.html#import-specifiers
The main thing is that you now have to include file extensions of all the files you import from yourself (so not for library imports)
It may seem a bit annoying at first, but your editor's auto-import should pick up on this, let me know if it doesn't and we can look into it. As well as following the spec, it makes it also much easier for bundlers like rollup to know what to do.

Secondly, you can no longer import foo from '@lblod/ember-rdfa-editor/foo'
As you can read in the link above, there are only 3 ways to specify an import path in es modules:

  • relative imports ../../foo.ts - they always work as expected
  • absolute imports 'file:///opt/nodejs/config.js' - I haven't actually tried these and don't know how they work, seems interesting
  • bare imports some-package/foo - These follow the nodejs resolution algorithm

The last one is the most interesting, since we now explicitly see node mentioned for the first time. I'm fairly certain this is not actually part of the esmodule spec, but rather a sort of "extension" that node provides by default, cause I had to enable a rollup plugin for it to work.

Here it is worth taking a step back to think what "the nodejs resolution algorithm" actually is. It's quite complex, and involves a number of heuristics and configurations which you may never have thought about, since it's designed to mostly "just work". But basically this is the part which knows to look in node_modules and how to navigate the files there. It is configured by the package.json files of both the importing package as well as the exporting package.

For us, this basically means: if the import is not relative, the algorithm will go look into the node_modules. This means that our imports which looked like @lblod/ember-rdfa-editor/foo no longer work. They were never a standard part of node, they were a convenience provided by the build pipeline (broccoli and webpack). We now no longer add that convenience (we technically could with a rollup plugin, but let's keep things as close to spec as we can)

What we do use however is the imports field of our package.json, which allows us to map specifiers that start with # to whatever we want. I've set this up such that we can use #root to refer to the src folder, so we can use imports like #root/commands/foo-command.ts.

Circular dependencies

dependency cycles are now explicitly disallowed and will result in a build error (I've explicitly configured it to error, rollup normally triggers a warning)
I haven't been able to find an explicit list of things that can go wrong with cycles, but all signs point to them being bad.

for development, this basically means: don't import from index files!
For that reason, I've disallowed importing from #root directly, although you could still get around that restriction using relative imports, but please don't.

the exports field

As mentioned above, the node resolution algorithm is configured through the package.json. This goes for packages we import, but also for packages that import us. Obviously we don't want the consuming app to have to remember where all the files are, or to add explicit file extensions. That's where the exports field in our package.json comes in. It is used by the node resolution algorithm when a package imports from us.

You may be a little shocked at how many entries we have. Unfortunately the globbing syntax is extremely basic and doesn't allow for much expressiveness, so we have to explicitly list each index file so that the algorithm knows to visit them (yes, /foo imports pointing to /foo/index was actually never part of any spec, it all worked cause of hacks and heuristics of build tools)

This also means that: think before making a new index file. It should significantly improve the importing experience of the consuming app by collecting a bunch of related exports. Don't just make one for the hell of it.

styles

styles and other assets work a little differently in a v2 addon. Before, you had the styles folder, and the broccoli pipeline would ensure that all the css (or sass, with the right plugin) there would get bundled along in such a way that the browser would load it.

In v2, there are no magic folders. Everything is managed through imports, starting from the entrypoints as defined in the rollup.config.mjs file. This also goes for css files, which are supported by default by the addon plugin that ember provides, but also sass, which are supported by rollup-plugin-sass.
If you look in the index.ts file, you can see the styles being imported there. They're still in the styles folder, but that folder is no longer special, and the style files can live anywhere.

NOTE: there is currently no scoping of the styles, so they are still global as soon as they're imported from any code path the app is running.

connected issues and PRs:

Setup

pnpm i
pnpm start
NOTE: take a moment to learn about --filter and how to work in pnpm monorepos https://pnpm.io/filtering

How to test/reproduce

Challenges/uncertainties

Still need to setup a good release script

Checks PR readiness

  • changelog
  • npm lint
  • no new deprecations

index files need to be explicitly mapped in the addon's export config
temporarily removed prosemirror-devtools to eliminate uncertainty
v2 addons can't inject config into the app build, so the app needs
to explicitly import the webpack config we provide
as such the v2 conversion will necessarily be breaking

Additionally, fixes errors for @glint/template imports by using
import type { foo } instead of import { type foo }

They are apparently different. The former can be purely stripped by
babel because it is specced as never triggering side-effects.
The latter does trigger side-effects, so babel can't fully strip the
line, and it turns into import "@glint/template"

This in turn errors in the app build, because "@glint/template" isn't
a real package because it contains only types. You'd wish something in
the chain would be smarter about this, but it isn't.

This essentially means the latter syntax is to be avoided at all times.
This is to avoid creating circular imports. Since index.ts exports
almost everything, it's really easy to create circles by importing from
it.
@abeforgit abeforgit force-pushed the internal/v2-addon branch 2 times, most recently from 78ba46c to f5a6925 Compare February 3, 2025 21:56
@abeforgit abeforgit force-pushed the internal/v2-addon branch 2 times, most recently from 53ca9bd to c1c4521 Compare February 4, 2025 01:02
@abeforgit
Copy link
Member Author

abeforgit commented Feb 6, 2025

still todo: downgrade sass a bit so we aren't flooded by deprecations from appuniversum

EDIT: done (by silencing the deprecations)

Copy link
Contributor

@elpoelma elpoelma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really nice! :)
A few things:

  • Am I correct in thinking the ember-cli-sass package is missing in the test-app?
  • Is there currently a reason why we should generate the css files?
  • Could we remove the option to do internal imports from index files altogether?
  • I'm not fully following what the vendor folder is for
  • Should we maybe exclude the the test-app package from changelogs?

Note: I did not test this yet with GN

"types": "./declarations/index.d.ts",
"default": "./dist/index.js"
},
"./webpack-config": "./dist/webpack-config.cjs",
Copy link
Contributor

@elpoelma elpoelma Feb 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this export still necessary?

Comment on lines +169 to +171
"imports": {
"#root": null,
"#root/index.ts": null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we map any index.ts file to null here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants