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

jssm does not work with TypeScript's Node16/NodeNext/Bundler moduleResolution #1295

Closed
tylerbutler opened this issue May 22, 2024 · 10 comments
Closed

Comments

@tylerbutler
Copy link

tylerbutler commented May 22, 2024

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:

src/index.ts:1:44 - error TS7016: Could not find a declaration file for module 'jssm'. '/home/tylerbu/code/jssm-node16/node_modules/jssm/dist/jssm.es6.mjs' implicitly has an 'any' type.
  There are types at '/home/tylerbu/code/jssm-node16/node_modules/jssm/jssm.d.ts', but this result could not be resolved when respecting package.json "exports". The 'jssm' library may need to update its package.json or typings.

1 import { from as createStateMachine } from "jssm";

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.

"exports": {
  "require": {
    "types": "./jssm.d.ts",
    "default": "./dist/jssm.es5.cjs"
  },
  "import": {
    "types": "./jssm.d.ts",
    "default": "./dist/jssm.es6.mjs"
  },
  "default": {
    "types": "./jssm.d.ts",
    "default": "./dist/jssm.es5.cjs"
  },
  "browser": "dist/jssm.es5.iife.cjs"
},

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.)

@tylerbutler tylerbutler changed the title jssm does not work with TypeScript's Node16/NodeNext moduleResolution jssm does not work with TypeScript's Node16/NodeNext/Bundler moduleResolution May 22, 2024
@StoneCypher
Copy link
Owner

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.

@StoneCypher
Copy link
Owner

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

@StoneCypher
Copy link
Owner

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.

@StoneCypher
Copy link
Owner

it may be that this is the underlying issue. that was already recommended to me by @mhsdev a few months ago for similar-ish reasons.

if so a repair is a relatively straightforward package.json change

i will try this later today and see if it helps

@tylerbutler
Copy link
Author

I use JSSM with nodenext every day and it seems to export types just fine for me

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:

src/index.ts:1:44 - error TS7016: Could not find a declaration file for module 'jssm'. '/home/tylerbu/code/jssm-node16/node_modules/jssm/dist/jssm.es6.mjs' implicitly has an 'any' type.
  There are types at '/home/tylerbu/code/jssm-node16/node_modules/jssm/jssm.d.ts', but this result could not be resolved when respecting package.json "exports". The 'jssm' library may need to update its package.json or typings.

1 import { from as createStateMachine } from "jssm";

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

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.

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.

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.

@StoneCypher
Copy link
Owner

Here's a minimal repro project: https://github.com/tylerbutler/jssm-node16

I appreciate the dialogue on the topic. I will do my best to fix this. Having a repro helps a whole lot.

 

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.

Understood.

 

Are you referring to the changes to add file extensions to the imports? E.g.

import { JssmError } from './jssm_error.js';

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.

 

I thought they were originally but maybe all that's needed is a types file and exports entry for each module kind.

Repro in hand, we will find out.

 

That said, my understanding is that at runtime, Node requires extensions - but maybe that's already addressed by by the rollup config.

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 build/ directory, which contains all major residues along the way, if you're curious.

@StoneCypher
Copy link
Owner

I am happy to open a PR, but I am having trouble with the tests, and I may have missed some steps in my local builds or something.

I would like to learn more about this. There should be no steps; it should be enough to write npm install && npm run build.

@tylerbutler
Copy link
Author

@StoneCypher I updated the description in this issue with a lot more detail, including a potential small fix using just the rollup config.

tylerbutler added a commit to microsoft/FluidFramework that referenced this issue May 28, 2024
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.
@StoneCypher
Copy link
Owner

@tylerbutler - with the merge of jssm/#569 and jssm-viz/#54, do you feel like this issue is close-able?

@tylerbutler
Copy link
Author

@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.

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

No branches or pull requests

2 participants