Skip to content
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

Merged
merged 11 commits into from
Dec 4, 2023
Merged

fix ESM types #200

merged 11 commits into from
Dec 4, 2023

Conversation

benmccann
Copy link

closes #199

Comment on lines 12 to 19
"import": {
".": "./build.mjs",
"types": "./index.d.mts"
},
"require": {
".": "./build.js",
"types": "./index.d.ts"
}
Copy link
Owner

@lukeed lukeed Nov 9, 2023

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 as index.d.mts
  • Add new index.d.ts which is the CommonJS export DTS file (to match "exports" description)
Suggested change
"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"
}

Copy link
Author

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? 😉

Copy link
Owner

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

Copy link
Author

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-commenter
Copy link

codecov-commenter commented Nov 9, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (84b50d0) 100.00% compared to head (dc55f67) 100.00%.

❗ Current head dc55f67 differs from pull request most recent head 8400ea3. Consider uploading reports for the commit 8400ea3 to get more accurate results

❗ 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.
📢 Have feedback on the report? Share it here.

Copy link
Owner

@lukeed lukeed left a 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 each package.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!

@lukeed lukeed merged commit a42c247 into lukeed:next Dec 4, 2023
5 checks passed
@lukeed lukeed mentioned this pull request Dec 4, 2023
@benmccann benmccann deleted the esm-types branch December 4, 2023 16:05
@lukeed
Copy link
Owner

lukeed commented Dec 4, 2023

Released in the next.24 batch

@benmccann
Copy link
Author

thank you Luke!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants