-
Notifications
You must be signed in to change notification settings - Fork 1
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
jssm does not work with TypeScript's Node16/NodeNext/Bundler moduleResolution #1295
Comments
I use JSSM with nodenext every day and it seems to export types just fine for me JSSM does not get its esm from typescript; it gets it from rollup. |
i notice that this PR also makes extensive changes to the project which have nothing to do with types, such as renaming every file in the project in a way that the typescript manual expressly says not to do |
this also seems to permanently add a CLI for a tool that this library doesn't use, and has incorrectly modified the testing coverage to add the themes for the documentation extraction there's a whole lot of stuff in this pr that didn't go mentioned i'm not comfortable editing the entire source tree by filename in a way the manual says is wrong. can you help me understand why you tried to make a change like this? i'm already off in a different pr evaluating that for months to see if it's actually valid. it's very surprising to see someone try to sneak that in without mentioning it. |
Here's a minimal repro project: https://github.com/tylerbutler/jssm-node16
Sorry, I am sure there are better ways to address this issue than what I did to work around it. I'm not very familiar with rollup or the way jssm is built and released. That's one reason I didn't open a PR, just this issue.
Are you referring to the changes to add file extensions to the imports? E.g. import { JssmError } from './jssm_error.js'; Those may not be necessary. I thought they were originally but maybe all that's needed is a types file and exports entry for each module kind. I vaguely remember trying to generate additional types and updating the export map but was out of my depth with rollup. That said, my understanding is that at runtime, Node requires extensions - but maybe that's already addressed by by the rollup config. |
I appreciate the dialogue on the topic. I will do my best to fix this. Having a repro helps a whole lot.
Understood.
Yes. This is a traditional torch fight in the typescript community. For what it's worth, I want the thing you did to be correct, and I hold this as a grievance against the TS team.
Repro in hand, we will find out.
Typescript provides extensions in its compile-downs. Rollup has that ability but said ability is not in use in this project. You can verify the process step by step in the |
I would like to learn more about this. There should be no steps; it should be enough to write |
@StoneCypher I updated the description in this issue with a lot more detail, including a potential small fix using just the rollup config. |
Updates the build-cli project to be ESM instead of CJS. For the oclif-related changes I followed this guide: https://oclif.io/docs/esm/#migrating-a-cjs-plugin-to-esm Summary of changes: - Ran ts2esm to add extensions to source file import statements. - Upgraded chalk, fs-extra, and node-fetch to more recent versions with better ESM support. - jssm's types aren't set up quite right for TypeScript to resolve them when using Node16/NodeNext module resolution, so I patched in support. I opened StoneCypher/fsl#1295 to discuss options for upstreaming the changes. - I've limited the patched files to package.json, rollup configs, and the two new rolled up declaration files. Critically, this means the patch has _no runtime changes_. The only changes are in the types and config files. This should make it almost impossible for there to have been runtime bugs introduced by these changes. - Updated the install build tools step in the CI workflows to use npm link to make flub globally available. I removed the hack shim we only used in CI. - Updated the syncpack config to account for new dependencies. - Regenerated the packlists. This should be done as a part of the build but it isn't yet so I updated them opportunistically. - Renamed the dangerfile to .cts so it's always CommonJS like it used to be. Seemed the safest approach. If we want it to be ESM we can make that change separately.
@tylerbutler - with the merge of jssm/#569 and jssm-viz/#54, do you feel like this issue is close-able? |
Yes! Thank you for the follow up. |
Describe the bug
jssm doesn't export types correctly for Node16+'s ESM module resolution. This means that using jssm with a project using moduleResolution: Node16 or NodeNext, which is what TypeScript recommends for Node projects, fails to compile because of missing types.
To Reproduce
Here's a minimal repro project: https://github.com/tylerbutler/jssm-node16
npm run build
in that project will invoke tsc and produces this error:See https://arethetypeswrong.github.io/?p=jssm%405.98.2 and you'll see that jssm doesn't work in either of the modern TypeScript moduleResolution settings because the types can't be resolved. arethetypeswrong is a great tool to investigate and validate your export map.
Possible fix
Create a rolled-up declaration for ESM and CJS using rollup-plugin-dts. This seems to work well in my testing and is the smallest fix I know of. I prototyped those changes and tested them on the repro project listed above and compilation worked as expected for both CJS and ESM. Prototype changes can be seen here: StoneCypher/jssm@main...tylerbutler:jssm:types-fixes-2
I didn't test the dev experience when looking up types. Now that there's a single type rollup getting back to "sourcefiles" might not work as it did.
Options that are tempting but don't work
You might be tempted to make the exports map look like this, and use the same declaration files for all exports. I.e.
This setup creates the following problems:
Expected behavior
jssm should work in modern TypeScript projects.
Other notes
When I investigated this, I noticed that the tsconfig used by jssm still has moduleResolution: Node (now known as Node10) and module: ES2015, which is known to produce ESM that won't work in Node. Likely you're not affected by that since you're using Rollup.
You could consider moduleResolution: Bundler as well, but Node16 will guarantee any ESM code generated by TypeScript will work in Node16. (This isn't an option right now since the bundler option was added in TS 5.0, so you'd have to upgrade TS too.)
The text was updated successfully, but these errors were encountered: