-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Move to ESM, remove index.d.ts
requirement
#186
base: main
Are you sure you want to change the base?
Conversation
Using |
By the way, this is 100% breaking change for programmatic API users. Perhaps #75 could be considered? Would be enough to return an object. This would make it easier to add any additional props in the future. The #75 was needed for
|
What's the benefit of using |
I think what you want is this: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#allowimportingtsextensions |
The ES module resolution is stricter about exports. Make sure it's actually exported correctly. |
The naming is unfortunate. |
Ok. We definitely want type checking to ensure they are valid. But we could also just run |
Let me know if anything is blocked on me commenting on something. |
I decided to go with that as well. I've been doing more locally, you're fine.
Re: #75, should we close that PR and open a new one? It's definitely stale, and the diff is pretty extraneous at this point. I can open a reminder issue. |
Yes |
Seems like the issue with the tests is that they are being transpiled to CJS, but since Related, is there a reason that we have |
|
I've uploaded some more changes (tests are still failing). I'll resolve the merge conflicts. A couple highlights: moved to |
Turns out this is because the TypeScript compiler is CJS: microsoft/TypeScript#32949 |
@tommy-mitchell Just a reminder that this is still open. If you're simply busy, feel free to ignore this. |
@sindresorhus this is on my todo list, I’ve got some local changes that are unpushed. If you have some time, I’d love your feedback on #196 |
); | ||
|
||
const combinedCompilerOptions = { | ||
...tsConfigCompilerOptions, | ||
...packageJsonCompilerOptions, | ||
}; | ||
|
||
const module = combinedCompilerOptions.module ?? ModuleKind.CommonJS; | ||
const module = combinedCompilerOptions.module ?? ts.ModuleKind.CommonJS; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const module = combinedCompilerOptions.module ?? ts.ModuleKind.CommonJS; | |
const module = combinedCompilerOptions.module ?? (pkg.type === 'module' ? ts.ModuleKind.Node16 : ts.ModuleKind.CommonJS); |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thoughts on these defaults?
function getModuleResolution(module: ts.ModuleKind): ts.ModuleResolutionKind {
if (module === ts.ModuleKind.NodeNext) {
return ts.ModuleResolutionKind.NodeNext;
}
if (module === ts.ModuleKind.Node16) {
return ts.ModuleResolutionKind.Node16;
}
if (module === ts.ModuleKind.ES2020 || module === ts.ModuleKind.ES2022 || module === ts.ModuleKind.ESNext) {
return ts.ModuleResolutionKind.Bundler;
}
// Fallback to Node16
return ts.ModuleResolutionKind.Node16;
}
index.d.ts
requirement
Ended up removing the
|
Are we still supporting testing types in CJS packages after moving to ESM? |
Seems like
|
Seems like there should be a way for us to just |
No |
Closes #183.
Closes #191.
Closes #175.
Checklist:
tsconfig.tsd.json
module
tonode16
target
toES2020
package.json
"type": "module"
AVA
v5.2.0, addtsx
for transpilationclean
script.js
to relative imports in source and test filesnode:
protocoltsd
's defaults (tsd doesn't catch missing extensions in declaration files #191)We should also update the assertion definitions to use
const
type parameters.Speaking of tests, trying to run
AVA
causes errors with@tsd/typescript
:In addition, updating
AVA
means thatt.throwsAsync
can also returnundefined
:cc: @sindresorhus