Support shipping Glint types with v2 addons

[simonihmig]

This issue is not a call to action, but meant to document and track the status quo of how to ship Glint types with v2 addons for better visibility here. I will try to keep it up to date, whenever new insights emerge...

We have already figured out and documented how an addon (v1) should ship its own Glint-based types. This should also be possible for a v2 addon, in the same manner (from the consumer's perspective). However a first experiment doing that was that successful yet (simonihmig/ember-stargate#494)...

The main problem was how to tell TS where to find the my-addon/glint module that an app is supposed to import (for the automatic registry registration of its components). While the v2 format does not require any kind of build step per se, most existing v2 addons have that (a pre-publish build step that is), and the v2 addon blueprint also has that baked in. The compiled artefacts will then usually live in a ./dist folder. Webpack (used by Embroider or ember-auto-import) will know how to resolve the imported modules to this folder on disk by making use of the exports feature of the addon's package.json. While this is well supported by Webpack and Node, only the upcoming TS 4.7 will add support for it. Which is needed to make the import of the addon types (my-addon/glint) work correctly. So the app would need to be on TS 4.7 to "just work", for older TS versions a manual paths entry in tsconfig.json would be required.

However even with 4.7 beta, I was not able to make it work (see linked PR above). My IDE would correctly resolve the imported Glint types, but glint itself apparently not, continuing to complain due to the addons's components not being added to the registry. 🤔 This needs further investigation...

[dfreeman]

Disclaimer: this is only based on reading that PR, not actually pulling if down and poking it with the typechecker, so I could be wrong! That said:

I think what you're hitting there isn't actually specific to Glint, but a more general issue that v2 TS addons are going to have to contend with for a while. Setting exports alone isn't enough to make the consumer aware of the path mapping: they also need to set moduleResolution: 'node16' in their tsconfig.json in order to opt in to honoring the exports field in their dependencies' package.json files. Anyone still using moduleResolution: 'node' will see TypeScript continue to try to resolve paths directly on-disk as before.

One way to work around this and support both moduleResolution: 'node' and moduleResolution: 'node16' in consumers for the time being would be with typesVersions. I may not have the notation exactly right, but I think something along these lines should work:

  "exports": {
    "./addon-main.js": "./addon-main.js",
    "./*": {
      "types": "./dist/*.d.ts",
      "default": "./dist/*.js"
    }
  },
  "typesVersions": {
    "*": {
      "*": ["dist/*"]
    }
  }

[simonihmig]

Hey Dan, this is very valuable feedback, thank you! I didn't realize that support for the exports key was coupled to node16, should have read the whole announcement post... 🙄

Will try out your suggestions and report back!

[simonihmig]

I tried this, but failed miserably, idk what's wrong...

I had TS updated, added exports, typesVersions and node16 etc., but this was not working. TS would basically not see the added registry items, complaining about ... can't be used to index type 'Globals'..

I then reverted the changes and tried to get to a working state with the most simple setup:

• using TS 4.6 and the normal node module resolution
• having an explicit paths config to point to the addon's glint.d.ts

Still I am getting the same errors, when running yarn lint:ts (alias for glint) inside the test-app.

Then added this commit to test what's happening:

• pageTitle is correctly typed, defined in the test-app's glint.d.ts
• but portal has an any type and TS complains about Property 'Portal' does not exist on type 'Registry', despite importing ember-stargate/glint (without errors, as it exists correctly in the addon's dist/glint.d.ts) which has the right registry declarations here
• when I copy paste the addon's glint.d.ts into the test-app (and fix the import paths), then it works correctly. As if it ignores the registry interface augmentation when coming from the external addon only!?

What am I missing? 😟

[dfreeman]

As if it ignores the registry interface augmentation when coming from the external addon only!?

This sounds similar to what you'd see with linked packages or another setup where the addon and host app have distinct copies of the package providing the registry. I'll pull the code from your PR later today and see if anything jumps out at me

[simonihmig]

This sounds similar to what you'd see with linked packages or another setup where the addon and host app have distinct copies of the package providing the registry.

@dfreeman Oh, yes, this was it! Thanks again! I accidentally had slightly different versions of glint deps (0.8.0 vs. 0.8.2) in my lockfile, for the v2 addon vs. the test-app. After deduping, everything works again as expected! 🎉

[simonihmig]

So, after adding proper exports¹ and typesVersions (and upgrading rollup-plugin-ts to the just released v3 to let babel do all the transpilation), things are working fine now, both with TS 4.6 and 4.7 (using node16)!

Closing this, as I would consider it a solved problem now. For reference, this is the passing PR again: simonihmig/ember-stargate#494

Footnotes

1. "default": "./dist/*.js" had to be changed to "default": "./dist/*", otherwise webpack wouldn't resolve a module when imported using a .js ending. ↩

[dfreeman]

Awesome, I'm glad you got it working!

"default": "./dist/*.js" had to be changed to "default": "./dist/*", otherwise webpack wouldn't resolve a module when imported using a .js ending

My understanding is that part of the point of exports is to abstract away the details of actual paths on disk (including file extension, whether that's .js, .mjs, etc) from your consumers. All the examples in the Node docs look like they add the file extension as part of the mapping: https://nodejs.org/api/packages.html#package-entry-points

[simonihmig]

Yes, that's how I am understanding it also...

I think what is going on is that the app reexports that the addon build is emitting (through https://github.com/embroider-build/embroider/blob/main/packages/addon-dev/src/rollup-app-reexports.ts) have a .js extension, like this:

export { default } from "ember-stargate/components/portal.js";

Which works usually. I didn't investigate this any deeper, but my guess is that when we have an exports mapping including a file extension like mentioned above, that webpack is trying to resolve this import on disk by looking basically at node_modules/ember-stargate/dist/components/portal.js.js.

Does that make sense? At least when I patched the app-reexports locally (just updated the dist/ files on disk after the addon build) to remove the file extension, then the webpack error was gone!

You can still see the webpack error here btw.

Maybe we need to make the rollup plugin strip the .js extension? /cc @ef4

[chriskrycho]

We must not strip the extension, or else it becomes invalid in Node’s resolution. It’s possible-to-likely we need to find and fix the issue in webpack instead. Edit: see below.

[dfreeman]

We must not strip the extension, or else it becomes invalid in Node’s resolution.

Extensions are only required for relative imports though, aren't they? My understanding is that for absolute (package-name-rooted) imports like the app reexports, you have to follow whatever pattern is established by exports, and it seems like the norm is not to include extensions.

[ef4]

We must not strip the extension, or else it becomes invalid in Node’s resolution

That's true as a default but it's under the control of each package's exports property. If a package wants consumers to import from extensionless paths, Node supports that. It's probably a best practice to do so.

[chriskrycho]

Ah, thank you both for the clarification/correction, you’re absolutely right. Previous comment rescinded!

[simonihmig]

If a package wants consumers to import from extensionless paths, Node supports that. It's probably a best practice to do so.

So, that means we can/should strip the extension from the reexport, right? (which would also make the v2 addon's reexport match what you get from ember g in a v1 addon)