-
-
Notifications
You must be signed in to change notification settings - Fork 1
Add dual package publishing support for ESM and CommonJS #2
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
Conversation
|
I appreciate the high quality PR, but I'm not sure I want to support/maintain CJS. Are you not able to use it as ESM? Also, I believe this will not correctly solve type resolution for CJS projects. See slevithan/regex#29
You went from one non-obvious but opinionated and intentional sort to a different non-obvious, opinionated sort. 🤷🏻♂️ What's the thinking behind it? |
…format revert to original package.json formatting
My colleague has struggled to make our particular NodeJS, TypeScript, and Jest combo work with your package for a few hours. We tried all the typical tricks found on Stack Overflow and hallucinated by LLMs, but no luck. 🤷🏻♂️ Forking your repo, adding CJS support, and switching to this version made my colleague happy immediately. 😅
You are correct. This is something I overlooked. In my initial MR, there was a problem: ❯ attw --pack
regex-recursion v6.0.2
Build tools:
- typescript@^5.7.3
- esbuild@^0.24.2
👺 Import resolved to an ESM type declaration file, but a CommonJS JavaScript file. https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseESM.md
┌───────────────────┬────────────────────────┐
│ │ "regex-recursion" │
├───────────────────┼────────────────────────┤
│ node10 │ 🟢 │
├───────────────────┼────────────────────────┤
│ node16 (from CJS) │ 👺 Masquerading as ESM │
├───────────────────┼────────────────────────┤
│ node16 (from ESM) │ 🟢 (ESM) │
├───────────────────┼────────────────────────┤
│ bundler │ 🟢 │
└───────────────────┴────────────────────────┘I committed a fix for this, and now it looks like so: ❯ attw --pack
regex-recursion v6.0.2
Build tools:
- typescript@^5.7.3
- esbuild@^0.24.2
No problems found 🌟
┌───────────────────┬───────────────────┐
│ │ "regex-recursion" │
├───────────────────┼───────────────────┤
│ node10 │ 🟢 │
├───────────────────┼───────────────────┤
│ node16 (from CJS) │ 🟢 (CJS) │
├───────────────────┼───────────────────┤
│ node16 (from ESM) │ 🟢 (ESM) │
├───────────────────┼───────────────────┤
│ bundler │ 🟢 │
└───────────────────┴───────────────────┘
Since my opinions on what a good-looking In my latest commit, I've reverted to your format.
That is, of course, for you to decide. We can live with a fork, no problem. It does look like this comes up from time to time. 😅 Again, up to you. |
You can probably avoid forking. I'm not super knowledgeable about CJS and build tools, but what I'd recommend is creating your own CJS bundle. That should be straightforward using e.g. If that doesn't work, one thing that definitely should is the following:
You should then be able to |
|
Makes sense. 🙌🏻 Thank you for taking the time. 🙇🏻♂️ |
|
Looks great! Nice readme. Glad that it's working for you now 😊 |
|
I've added a link to your wrapper library in the readme: 0396ff3. |
|
Thank you! 🙌🏻 |
This MR supports dual package publishing, allowing the package to be consumed by ESM and CommonJS projects without any breaking changes.
Changes
package.json🙈Benefits
require()