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: support cjs #301

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

acheronfail
Copy link

@acheronfail acheronfail commented Oct 8, 2024

It doesn't take much to support CJS, and it really helps the community with large applications and projects that continue to use this package.

Explanation

I'm not going to put my opinions on ESM vs CJS here since I don't want to start a flame war, but this change should make both parties happy, by supporting both: superjson can still be written in ESM and ship ESM, but tsc will transpile it to CJS for the CJS consumers.

If the package is require()'d, then it will use the dist-cjs/* files, and if it's import'd, then it will use dist/index.js.

Testing

  1. clone and setup superjson locally
  2. run yarn link inside superjson's directory
  3. create a node package with "type": "module", in this directory run yarn add superjson && yarn link superjson
  4. in this new directory, create index.js with import S from 'superjson'; console.log(S); and run it
  5. create a node package with "type": "commonjs", in this directory run yarn add superjson && yarn link superjson
  6. in this new directory, create index.js with console.log(require('superjson'));, and run it

Maintenance burden

In terms of burden for superjson maintainers, really it's having an extra 2 files: tsconfig.cjs.json and commonjs.js. Both of these files together add up to 33 lines of code - it's not a lot. Since the superjson package uses ESM, we can confidently just patch the require calls in the tsc output, since ESM imports are more strict than CJS ones, etc.

Final notes

Ultimately this is up to the superjson maintainers to accept or not, but I've at least proposed it as a starting point. Up to the maintainers to decide if they want to do it this way to support all their consumers or just a subset...

Fixes #268
Fixes #299

@acheronfail acheronfail requested a review from Skn0tt as a code owner October 8, 2024 22:05
@acheronfail acheronfail marked this pull request as draft October 8, 2024 22:16
@acheronfail acheronfail force-pushed the support-cjs branch 2 times, most recently from 7e4d143 to 7965849 Compare October 8, 2024 22:26
@acheronfail acheronfail marked this pull request as ready for review October 8, 2024 22:27
half0wl added a commit to half0wl/superjson-cjs that referenced this pull request Nov 29, 2024
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.

Feature request: add commonjs and esm exports Error [ERR_REQUIRE_ESM]: require() of ES Module
1 participant