Changed moduleResolution to NodeNext #42
Conversation
✅ Deploy Preview for twoslash ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -93,7 +93,7 @@ export default defineConfig({ | |||
| } as any | |||
|
|
|||
| try { | |||
| const jiti = JITI(id, { | |||
| const jiti = (JITI as any)(id, { | |||
There was a problem hiding this comment.
This was erroring after this change, but I'm unsure why.
| @@ -5,7 +5,7 @@ | |||
| "private": true, | |||
| "packageManager": "[email protected]", | |||
| "scripts": { | |||
| "typecheck": "tsc --noEmit", | |||
| "typecheck": "tsc", | |||
There was a problem hiding this comment.
Putting noEmit inside the tsconfig.json is a best practice, since users of this repo won't always know to run pnpm typecheck and not just pnpm tsc.
| @@ -7,8 +7,8 @@ | |||
| "esnext" | |||
| ], | |||
| "moduleDetection": "force", | |||
| "module": "esnext", | |||
| "moduleResolution": "node", | |||
| "module": "NodeNext", | |||
There was a problem hiding this comment.
Feel free to change this to Node16 if you like for 'stability', but I think NodeNext is perfectly fine.
There was a problem hiding this comment.
I would always prefer "Bundler" - I wonder if there is any downside to it?
There was a problem hiding this comment.
Yes, Bundler wouldn't have caught my runtime error, because it wouldn't have prompted you to add the .js to the end of the file.
There might be something you can tweak in your unbuild settings to make this work, but NodeNext is always going to be safer and more predictable than Bundler for libraries.
There was a problem hiding this comment.
Ah, I see the problem now. But I am honestly not a fan of the .js extension, especially when all files are actually .ts. Is it possible to have a middle ground where the library extension error can be caught but not force every import to have .js?
There was a problem hiding this comment.
I'm not sure - the only way to trigger this warning is with moduleResolution: NodeNext. I wouldn't advise messing around with allowImportingTsExtensions, since it would probably trigger the same runtime error for me if not transformed.
I'm also not sure whether rollup could catch this error. It has no idea whether typescript/lib/tsserverlibrary is a file (which it is) or a package entrypoint defined via exports. So, it doesn't add the .js extension in the output.
I have to say that having been using the .js extension for a while, it is really, really nice. So nice to know exactly which file is being targeted and keep you in the mindset that you're writing JS, not TS.
There was a problem hiding this comment.
Also for types-only imports it’s technically correct. In this case not because of Node.js, but because TypeScript dictates this.
In TypeScript 5.3 typescript/lib/tsserverlibrary.js was made an alias of typescript. So all 'typescript/lib/tsserverlibrary' imports can be replaced with 'typescript'.
There was a problem hiding this comment.
@antfu Could we merge this, then look for a better solution afterwards? I am currently blocked by this, so having a patch would be pretty useful.
There was a problem hiding this comment.
@mattpocock Sorry, I am on a conf and wasn't very responsive. I am still not sure .js is a long-term solution for me (unless @orta has some preference). It's nice as @remcohaszing pointed out - could we have a smaller PR that changes typescript/lib/tsserverlibrary to typescript to fix the runtime error, without adding .js for now?
There was a problem hiding this comment.
Instead of adding extensions manually to all the files, a plugin like https://github.com/favware/esbuild-plugin-file-path-extensions can be created for unbuild to add the extensions at build time the you can use Bundler module resolution
|
Switched to Thanks for bringing this up. |
twoslash-cdn (and possibly other packages in the repo) can't currently be run in a Node.js environment.
This is mainly due to a single import.
So, I updated the root tsconfig.json to use NodeNext, and updated all import paths to follow it.
This will solve my downstream issue.
LMK if you want me to raise an issue to link to, but I figured this was a quick win.