Skip to content

Ladle breaking TypeScript when moduleResolution: "NodeNext" #267

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

Closed
frehner opened this issue Sep 28, 2022 · 9 comments · Fixed by #275
Closed

Ladle breaking TypeScript when moduleResolution: "NodeNext" #267

frehner opened this issue Sep 28, 2022 · 9 comments · Fixed by #275
Labels
bug Something isn't working

Comments

@frehner
Copy link
Contributor

frehner commented Sep 28, 2022

Describe the bug

Ladle breaks Typescript type-checking (e.g. tsc --noEmit) when Typescript has the config moduleResolution: "NodeNext", even when skipLibCheck: true

Reproduction

https://stackblitz.com/edit/ladle-zq9ane?file=package.json

and run

npm run typecheck

The specific error that causes issues looks like the following, and note that it's looking in node_modules:

node_modules/@ladle/react/lib/app/exports.ts:2:33 - error TS2835: Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node16' or 'nodenext'. Did you mean './src/context.js'?

2 import { useLadleContext } from "./src/context";

Environment

  • OS: MacOS
  • Browser: n/a
  • Version: n/a

More Context

From this TS issue (that apparently is "working as intended" for now), it seems that the error comes from the fact that Ladle's package.json#types field points to a .ts file instead of a .d.ts file.

@frehner frehner added the needs triage needs to be reviewed label Sep 28, 2022
@gunters63
Copy link

gunters63 commented Oct 5, 2022

Yes, we have the same problem (with node16 resolution). We just don't typecheck (yet) in our docs projects.
Good that in our monorepo Ladle-dependent code lives only in separate docs projects (for component libraries), so its not a big issue.

I think a d.ts. file in the exports won't work because there are no separate .d.ts Files, only .ts files. This works because the files in Ladles lib directory are supposed to be processed by Vite/Esbuild all the time.

I am not sure how this problem can be solved. I experimented a bit by putting a local tsconfig.json in the ladle folder in node_modules and set "moduleResolution": "node" there, but that didn't work.

@frehner
Copy link
Contributor Author

frehner commented Oct 5, 2022

Yeah, Ladle needs to generate .d.ts files for when it's published to npm, I don't think there's any way around that.

Fortunately it maybe isn't too hard to fix: add a tsc --emitDeclarationOnly before packaging up the library, and update the package.json to point to the .d.ts file that's generated. Maybe. 🙂

@gunters63
Copy link

gunters63 commented Oct 5, 2022

@frehner : I think this will not be enough. Typescript never touches the imports, the Node16/NodeNext module resolution would still require the .js extension for relative imports. Pointing package.json to the .d.ts would also not help this.

@gunters63
Copy link

Maybe an easy solution would be to append .js to all imports in the Ladle sources. This could be backwards compatible to the "legacy" Node resolution. Node16/NodeNext resolution could also maybe set in tsconfig.json to make sure all imports are correct.

@frehner
Copy link
Contributor Author

frehner commented Oct 5, 2022

@frehner : I think this will not be enough. Typescript never touches the imports, the Node16/NodeNext module resolution would still require the .js extension for relative imports. Pointing package.json to the .d.ts would also not help this.

I'm only talking about Ladle creating .d.ts files next to the .ts files in its own library, right before publishing to npm. It should work with that change I believe.

@tajo
Copy link
Owner

tajo commented Oct 6, 2022

I'm only talking about Ladle creating .d.ts files next to the .ts files in its own library, right before publishing to npm. It should work with that change I believe.

Can you test this out? That seems like a simple fix if it works.

@tajo tajo added bug Something isn't working and removed needs triage needs to be reviewed labels Oct 6, 2022
@frehner
Copy link
Contributor Author

frehner commented Oct 6, 2022

@tajo I have it working - but ONLY for external packages (tested using linking).

However, because the types are only generated right before creating the npm package and aren't kept in source control, that means that other packages in the ladle monorepo can't find the types and fail the typecheck step.

So I think you have a couple of options here:

  1. Generate the .d.ts and keep them as part of the package / VCS / git history, so that they're always there
  2. Change the library so that you compile the TS to .js and .d.ts (most libraries do things this way)
  3. Other ideas?

Would it be helpful if I put up my changes so you could see them, even if it's not fully working yet?

@tajo
Copy link
Owner

tajo commented Oct 6, 2022

Generate the .d.ts and keep them as part of the package / VCS / git history, so that they're always there

This is fine with me since we don't change the exported API that often anyway. As long as we have a CI test for it to make sure it's in sync.

Change the library so that you compile the TS to .js and .d.ts (most libraries do things this way)

This would mean flipping all the code (including node/CLI) into .ts? I really prefer the setup where the node code doesn't need an extra compilation step and types are defined through JSDoc.

@frehner
Copy link
Contributor Author

frehner commented Oct 6, 2022

This would mean flipping all the code (including node/CLI) into .ts?

No, not necessarily. They could remain as-is, if you wanted to go this route. You would only have to process the .ts files.


Actually, now that I think about it more, we don't necessarily have to put them into git / VCS:

Option 3, we could have the .d.ts files output to a folder that's ignored by .gitignore, and then would need to add an additional step (to contributing.md) when you spin up the project to generate the types files. Could maybe even run the type generation in a "postinstall" npm hook (for ladle monorepo support) and in a "prepack" npm hook (so they're included in the npm package/bundle).

[edit] I don't think the "postinstall" hook would work, actually, as that also runs when a package is installed by another package.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants