-
-
Notifications
You must be signed in to change notification settings - Fork 174
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
fix ESM types #200
fix ESM types #200
Conversation
packages/cluster/package.json
Outdated
"import": { | ||
".": "./build.mjs", | ||
"types": "./index.d.mts" | ||
}, | ||
"require": { | ||
".": "./build.js", | ||
"types": "./index.d.ts" | ||
} |
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.
For all modules, please make these changes:
- TS says its requires "types" to be first
- Rename "." -> "default" & order it after "types"
- Rename
index.d.ts
asindex.d.mts
- Add new
index.d.ts
which is the CommonJS export DTS file (to match "exports" description)
"import": { | |
".": "./build.mjs", | |
"types": "./index.d.mts" | |
}, | |
"require": { | |
".": "./build.js", | |
"types": "./index.d.ts" | |
} | |
"import": { | |
"types": "./index.d.mts", | |
"default": "./build.mjs" | |
}, | |
"require": { | |
"types": "./index.d.ts", | |
"default": "./build.js" | |
} |
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.
Thanks for the quick review Luke! I've made the package.json
changes you suggested. I'm not sure I understand the last bullet point or how it's different from what exists. Are you suggesting to use export =
in case the user has esModuleInterop: false
?
Any chance you'd consider dropping the CJS build? 😉
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.
Are you suggesting to use
export =
Yes, to lean into the CommonJS TS definitions, rather than relying on tsc's implicit conversion
Any chance you'd consider dropping the CJS build? 😉
No
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.
Ok, I just pushed that change. I don't have a lot of experience with that style. I spent like an hour tying to find documentation and examples for it and it's not used much as far as I could see. Hopefully I did it correctly. If I'm way off base I think we could just revert the last commit. I feel it's a bit out of scope to update the types for CJS users and we could probably leave them as is in this PR. I was really just trying to get the ESM types updated, which I'm a lot more familiar with.
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## next #200 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 204 204
=========================================
Hits 204 204 ☔ View full report in Codecov by Sentry. |
- named exports only
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.
Thanks Ben :)
The last remaining changes I took care of were:
- updating the
files
lists for eachpackage.json
file so that any new*.d.mts
files would actually be sent to npm - wrapping the CommonJS exports into a namespace so that no exported types were lost
- Point ESM/CJS types to the same file if & when the module only has named exports. Publint may still print a warning in this case, but that's fine :) CJS <-> ESM interop is totally fine when dealing with named exports only.
Sorry for the delay. Thanks for the work, patience, and ping!
Released in the |
thank you Luke!!! |
closes #199