Skip to content

Conversation

@tao-cumplido
Copy link
Contributor

@tao-cumplido tao-cumplido commented Nov 14, 2024

Fixes #28

TS resolution failed for CJS packages as shown by Are The Types Wrong.

This fix changes the bundle outputs to dist/module/regex.js and dist/commonjs/regex.js for ESM and CJS respectively and adds a postbuild script which does two things for each dist type:

  • copy the generated types into the dist folder
  • generate a package.json file with only the type field set

Running npx attw --pack . now produces the following result:

regex v4.4.0

Build tools:
- @arethetypeswrong/cli@~0.17.0
- typescript@~5.6.3
- esbuild@^0.24.0

💀 Import failed to resolve to type declarations or JavaScript files. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/NoResolution.md

⚠️ A require call resolved to an ESM JavaScript file, which is an error in Node and some bundlers. CommonJS consumers will need to use a dynamic import. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/CJSResolvesToESM.md


┌───────────────────┬──────────┬──────────────────────────────┐
│                   │ "regex""regex/atomic"               │
├───────────────────┼──────────┼──────────────────────────────┤
│ node10            │ 🟢       │ 💀 Resolution failed         │
├───────────────────┼──────────┼──────────────────────────────┤
│ node16 (from CJS) │ 🟢 (CJS) │ ⚠️ ESM (dynamic import only) │
├───────────────────┼──────────┼──────────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM) │ 🟢 (ESM)                     │
├───────────────────┼──────────┼──────────────────────────────┤
│ bundler           │ 🟢       │ 🟢                           │
└───────────────────┴──────────┴──────────────────────────────┘

It still errors for regex/atomic with a node10 module option, but I believe this is irrelevant since the exports field and subpaths are not supported by Node 10 anyway.

I tried to integrate the attw check into the test script but it would deadlock since attw calls npm pack which in turn calls npm prepare which calls npm test.

Edit:
Forgot to mention that I changed the TS dependency to patch range since TS is known for not following semver and introducing breaking changes in minor versions.

typescript is known to not follow semver and introduce breaking changes in minor versions
@slevithan
Copy link
Owner

slevithan commented Nov 14, 2024

Thanks for the PR! This is very helpful since CJS and TS are both moderately unfamiliar areas for me.

It's unfortunate that this added complexity is needed. I'd almost rather go ESM only, but I guess this is the right path. I'm curious, are you personally trying to use this library from a CommonJS module, or just helping out?

generate a package.json file with only the type field set

Can you confirm that this is necessary for it to work? Can import types and require types not point to the same file?

I tried to integrate the attw check into the test script but it would deadlock since attw calls npm pack which in turn calls npm prepare which calls npm test.

Would changing the current prepare to instead use prepublishOnly enable adding attw to test? If so, I think that would be worthwhile.

@tao-cumplido
Copy link
Contributor Author

It's unfortunate that this added complexity is needed. I'd almost rather go ESM only, but I guess this is the right path. I'm curious, are you personally trying to use this library from a CommonJS module, or just helping out?

Yeah, TypeScript support for dual packages does feel unnecessarily complex. I'm trying to use regex in a library which I publish as a dual package myself. I'd love to drop CJS support but I'm currently keeping it because VSCode extensions need to be shipped as CJS. Node 24 will likely ship with the ability to load ESM modules via require() calls from CJS, maybe it'll also get backported. So when VSCode ships with a Node version that supports this I'll most likely drop CJS support, or when they support extensions to load as ESM.

Can you confirm that this is necessary for it to work? Can import types and require types not point to the same file?

Unfortunately it is necessary for TypeScript's resolution algorithm. I am not very good at explaining stuff, but I'll try as best as I can: So, in the case of regex the package.json's type field is set to module, i.e. Node will load .js files as ESM by default. Import statements in the sources with .js extensions are kept as is by TS. When TS tries to resolve the library from a CJS package it will encounter the exports entry pointing to a .cjs file and type definitions for ESM because of the type: module field. That's why Are The Types Wrong list the issue "Masquerading as ESM". Explicit CJS type definitions would require .d.cts files. Unfortunately, as far as I'm aware, TS is unable to produce both .d.cts and .d.mts from a single source. That's why tools like tshy exist.
Now, nested package.json files in the dist folder for each type is a possible solution to the problem. When loading JS files, Node will use the closest package.json to determine if it's ESM or CJS. That way using simply .js and .d.ts extensions works as expected.
I hope that was somewhat clear, I know it's a mess. 😅

Would changing the current prepare to instead use prepublishOnly enable adding attw to test? If so, I think that would be worthwhile.

I think so, will try it and update.

Do you intend to keep support for Node 14? It's EOL for over a year now. If so I will have to address the failing CI check.

change prepare script to prepublishOnly: attw calls npm pack which in turn would call prepare and then deadlock
@tao-cumplido
Copy link
Contributor Author

I changed prepare to prepublishOnly and added attw to test. I limited attw to the default entrypoint since the ./atomic entrypoint would still error and is not documented anyway.

@slevithan
Copy link
Owner

slevithan commented Nov 15, 2024

Thanks for all the details!

Do you intend to keep support for Node 14? It's EOL for over a year now. If so I will have to address the failing CI check.

What would it take to address the failing check? And which version of Node first supports the fs.cp function you're using?

I don't particularly care about Node 14, but I like running tests on it because it's the first version that supports all the JavaScript features I use within the library (e.g. including the ?. operator), so testing with it ensures I'm not changing the list of 2020-era browsers that this library supports (without the need for a user-side transpilation step).


Heads up that this PR will be a breaking change due to removing ./types from package files. But I need some other minor breaking changes for work I'm doing to support Oniguruma-To-ES anyway, so hopefully I can publish v5.0 shortly after this PR lands.


Edit: Based on this thread, fs.cp was added in Node 16.7, and it looks like maybe the best alternative that isn't unmaintained is fs-extra. Okay, why don't we go ahead and test on Node 16 instead of 14 in order to make this work as is.

@slevithan slevithan merged commit dfac903 into slevithan:main Nov 15, 2024
1 of 2 checks passed
@slevithan
Copy link
Owner

Some minor-ish follow-ups in d18b974, including moving CI to Node 16.

@slevithan
Copy link
Owner

Now published in v5.0.0.

@slevithan
Copy link
Owner

For future reference, here's one ATTW-passing example that generates separate d.cts and d.mts files, and uses the typesVersions property in package.json.

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.

TypeScript resolution fails when trying to import the package from a CJS module

2 participants