attempt fix typescript modules error for runno runtime

[jonathanpv]

maybe am wrong lmk

related:

• Recommended way to get type declarations in a modern vite / typescript project #323

[taybenlor]

This seems right, but I can't find good documentation on what the correct thing to do is.

This stackoverflow link suggests this approach is not enough, and the import and require need to be split: https://stackoverflow.com/questions/58990498/package-json-exports-field-not-working-with-typescript

Can you find some documentation on this?

We should also update all package.json files in the repo if this change is necessary, not just this one.

[agilgur5]

Can you find some documentation on this?

I can help answer this 🙂

TS's docs on publishing types are here: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html. They seem to not have been updated¹ with regard to package.json#exports (aka Node.js "export conditions"), but the TS 4.7 release notes detail it: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#packagejson-exports-imports-and-self-referencing. The compilerOptions documentation also has some details on the consuming side: https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-node18-nodenext.

Node's own docs on this are here: https://nodejs.org/api/packages.html#exports. That links to more detailed docs that involve all the different permutations. Given the current usage, "conditional exports" and "subpath exports" are particularly relevant.

This stackoverflow link suggests this approach is not enough, and the import and require need to be split: https://stackoverflow.com/questions/58990498/package-json-exports-field-not-working-with-typescript

Normally this is specifically in the case where you have different, "conditional" exports for CJS (require) vs MJS (import) formats.
The TS 4.7 release notes appear to add additional stipulations here:

It’s important to note that the CommonJS entrypoint and the ES module entrypoint each needs its own declaration file, even if the contents are the same between them. Every declaration file is interpreted either as a CommonJS module or as an ES module, based on its file extension and the "type" field of the package.json, and this detected module kind must match the module kind that Node will detect for the corresponding JavaScript file for type checking to be correct. Attempting to use a single .d.ts file to type both an ES module entrypoint and a CommonJS entrypoint will cause TypeScript to think only one of those entrypoints exists, causing compiler errors for users of the package.

So this answer appears to be most correct, where they are split.
Why adding a single subpath types export worked may be for a few reasons:

1. usage of "moduleResolution": "bundler" (per Recommended way to get type declarations in a modern vite / typescript project #323 (comment)) and differences between Vite / esbuild and tsc
2. It might only cause an issue if you try to simultaneously do a CJS import; i.e. the error condition might not have been hit and this might be sufficient in most cases
3. other wackiness, non-spec approaches, graceful fallbacks, etc; I'm honestly not sure

Also, in my experience, if you don't have subpath exports, then plain top-level types seems to work fine. So the subpath "." may be contributing to this. EDIT: nvm, see below comment

---

If you're confused by this, well, I feel like I get confused every time I look at this and I help maintain a TS compiler integration (and maintained and contributed to lots of other TS tooling in the past, including the TS docs) 😅
That being said, it is easier to grok the less features you use; conditional exports, subpath exports, and TS support all add complexity to the scope, especially when used together, and all the fallbacks for backward-compat add some more too.

To add to the confusion, Node's support for ESM in the module VM API was still experimental last I checked and so was blocking test runners like Jest jestjs/jest#9430 from natively supporting it too (there are workarounds)

Tbh, it's a lot easier to only support newer, LTS versions of Node that all support ESM as a standard spec (see also https://deno.com/blog/commonjs-is-hurting-javascript, https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c). Backward-compat is hard and confusing, so big respect to all those who attempt it, and I extensively try to do so too, but even I'm planning to move my supported libraries to pure ESM in breaking changes eventually to simplify builds and reduce the maintenance burden, especially as older Node is now out-of-support.

Footnotes

1. There appears to be an existing issue for the outdated publishing docs: https://github.com/microsoft/TypeScript-Website/issues/3088 ↩

[taybenlor]

Thanks so much @agilgur5 - those links were very useful.

Okay so it seems like the right fixes are either:

• add deeper settings inside the "exports" field to provide the types and the default for each of require and import

Or

• delete the "exports" field

Is that right? If that's the case deleting seems to be the right move.

[agilgur5]

Hey sorry for the delayed response, I had a pretty bad flare-up in my medical condition the past ~2ish weeks and so haven't really touched a computer much at all in that timeframe 😕 Feeling better overall now, but not fully back yet.

removing exports field / removing CJS support

Or

• delete the "exports" field

Is that right? If that's the case deleting seems to be the right move.

If what you mean by that is removing CJS support, then yes. That is a breaking change though, so would require a major bump. This fix of adding exports.types could be "good enough" in the interim

Technically, you can still support CJS builds in other ways, like the older main, module, umd:main, etc options (some of which you already have), but other than main, those were all "unofficial" / non-spec standards, so do not necessarily work the same everywhere

setting types in exports

• add deeper settings inside the "exports" field to provide the types and the default for each of require and import

Also, in my experience, if you don't have subpath exports, then plain top-level types seems to work fine. So the subpath "." may be contributing to this.

I do have an update on this as I coincidentally got an identical fix in one of my repos in the same timeframe: agilgur5/react-signature-canvas#126. My statement is above is incorrect for newer TS versions with newer moduleResolution settings (e.g. node16, nodenext, bundler). I apparently had older options in my dev environments and root-caused it in agilgur5/react-signature-canvas#126 (comment).
Copying here, TS maintainers stated in microsoft/TypeScript#52363 (comment) and microsoft/TypeScript#46334 that the top-level types field is indeed not used as a fallback when exports is defined. One of the maintainers also created https://github.com/arethetypeswrong/arethetypeswrong.github.io (website and CLI) to help diagnose these package.json types issues.

2. It might only cause an issue if you try to simultaneously do a CJS import; i.e. the error condition might not have been hit and this might be sufficient in most cases

Given these new findings and some testing I did, this seems to be the most accurate answer afaict, and matches how Node works in the general case in my experience.
So this change is likely sufficient for the vast majority of usage; likely only mixed ESM + CJS codebases would still have a problem. For those, you'd need to duplicate the types in require and import with two distinctly named, but identical files -- same as the SO answer previously linked above.

recommendation

Given all of the above, personally, I'd recommend making this change to all the relevant package.json in this repo and releasing patch versions. If the mixed ESM + CJS issue ever pops up, you can do the more complicated duplication fix in another patch.
Then you can deprecate CJS support and remove it in a breaking change / major version bump.
This is what I'll be doing myself in my own packages.

We should also update all package.json files in the repo if this change is necessary, not just this one.

@jonathanpv this update would still be necessary to your PR if Ben goes with this option. I can make a superseding PR if both of you prefer.

[jonathanpv]

hey sorry! I need some help with this, I dont understand many of the links sent and how I would begin to modify do I need to close the PR so someone else can take lead?

[taybenlor]

@jonathanpv from a quick read @agilgur5 is suggesting that your fix in this PR is correct, and that to ship it we'll need to update each of the packages in Runno.

@agilgur5 is that right?

If so could you please make the same change in the other packages? (see the packages folder at the root of the repo)

[agilgur5]

@agilgur5 is that right?

Essentially¹ yes 👍

do I need to close the PR so someone else can take lead?

no, anyone can file a superseding PR, but I figure you might want to update it yourself and get this merged as the original author and bug finder!
If not, I can add you as co-author on a superseding PR 🙂

Footnotes

1. More accurately that the fix is "good enough" and likely sufficient for the vast majority of usage, so I personally recommend it at this time given the trade-off of complexity vs necessity/usage of a more correct, deeper fix ↩

[taybenlor]

This has been superceded by #333 and has been released as v0.7.1 - please tell me if it now works for you.