✨ Switch to nodenext resolution #1026
Conversation
coyotte508
requested review from
SBrandeis,
gary149,
Wauplin,
julien-c,
pcuenca and
ngxson
as code owners
There was a problem hiding this comment.
This was the only way I found to have at the same time:
- .d.cts and .d.ts generation
- "go to source" still working
It's also possible to upgrade tsup and use --dts (instead of tsc) to generate the declaration files, but then the "go to source" won't work.
There was a problem hiding this comment.
wow that feels like kind of a dubious method tbh ^^'
iiuc from https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext, the most correct way to get cjs types would be to call tsc --emitDeclarationOnly --declaration --outDir xxx after temporarily removing type: "module" from the package.json and then rename+move those declaration files; did you try that by any chance?
There was a problem hiding this comment.
I don't think you need to edit the package.json, you can run tsc with --module commonjs maybe.
But anyway I think the types are the same, only the js flies would be different. And with your solution we still need to rename / edit the .ts and .map files.
Thinking about it, we should not use tsup at all for the sourcemaps to work correctly. and in that case we do indeed need to run tsc twice. 🤔
But it was already broken, problaby
There was a problem hiding this comment.
cc @andrewbranch too on this btw, in case he has some insights for us by any chance on the best way to emit both ESM and CJS types through tsc directly (while transpiling the js itself through tsup) 🙏
There was a problem hiding this comment.
I don't think you need to edit the package.json, you can run tsc with --module commonjs maybe.
well, the thing is, it's not the same (from the link shared in my previous message):
CommonJS emit is similar to --module commonjs, but dynamic import() calls are not transformed.
^^ I guess this doesn't affect declaration files though, so maybe
There was a problem hiding this comment.
we don't have dynamic import calls anyway in @huggingface/tasks 🤓
(to keep in mind for @huggingface/hub though)
There was a problem hiding this comment.
https://github.com/isaacs/tshy automates this in a safe way
There was a problem hiding this comment.
tysm for the pointer, we'll check it out!
| @@ -9,10 +9,9 @@ | |||
| }, | |||
| "main": "./dist/index.cjs", | |||
| "module": "./dist/index.js", | |||
| "types": "./dist/src/index.d.ts", | |||
| "types": "./dist/index.d.ts", | |||
There was a problem hiding this comment.
I'm curious, do we need this at all? (given it's resolved automatically based on the require/import values anyway, iiuc)
There was a problem hiding this comment.
(I also see the second option at https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md#common-causes doesn't have a types entry at all)
There was a problem hiding this comment.
I don't know - maybe for non-node / older contexts?
There was a problem hiding this comment.
wow that feels like kind of a dubious method tbh ^^'
iiuc from https://www.typescriptlang.org/docs/handbook/modules/reference.html#node16-nodenext, the most correct way to get cjs types would be to call tsc --emitDeclarationOnly --declaration --outDir xxx after temporarily removing type: "module" from the package.json and then rename+move those declaration files; did you try that by any chance?
yes sorry didn't want more merge conflicts with all the imports 🙈 |
Follow up to #1026 Benefit is that exports are better defined, and source-mapping works. Also, move the generation scripts from `@huggingace/tasks` to `@huggingface/tasks-gen` cc @Wauplin @SBrandeis So that the dev dependencies are not tacked on `@huggingface/tasks`. Some package managers, *cough* yarn *cough* like to download them when importing a package, even if they're irrelevant.
Seems necessary for #1025 - also handle #1001 (only for tasks at the moment) cc @martin-gorner
also so that https://arethetypeswrong.github.io/?p=@huggingface/tasks is ✔️ cc @Pierrci - in addition to the .cts exports
And finally export snippets types (https://github.com/huggingface-internal/moon-landing/pull/10998/files#r1841375306)
Types pass: https://arethetypeswrong.github.io/?p=%40huggingface%2Ftasks%400.13.0-test5 !