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

Ts esm commonjs support #17

Closed

Conversation

alexfigliolia
Copy link

Hey there! I came across this repo and thought it could save a me a little time in spinning up a solution of my own. I noticed that the code is common.js now living in a mostly es-modules/typescript world. I took a sec to setup build compatibility for all three formats and figured I'd PR it if you're interested.

When making a release to npm, calling yarn build will compile separate builds for es-module, common-js, and typescript projects. The distribution of the correct build to the correct project is handled through the project's package.json which now contains the following:

{
  "files": [
    "lib",
    "dist"
  ],
  "exports": {
    ".": {
      "import": "./dist/mjs/index.js",
      "require": "./dist/cjs/index.js",
      "types": "./dist/types/index.d.ts"
    }
  },
  "main": "dist/cjs/index.js",
  "module": "dist/mjs/index.js",
  "types": "dist/types/index.d.ts"

You'll notice in the "files" declaration, I'm now serving both the generated dist directory as well as the source files.

@alexfigliolia
Copy link
Author

Apologies by the way for the large diff, I had to switch linters and figured I'd add formatting while I was at it. To summarize the necessary portions of the changes:

  1. I ported the lib/gzip-static module over to typescript. This required no logical changes, but a few type-anotations
  2. I ported the test/support/http module over to typescript. This did require refactoring prototypal inheritance over to class based. I could have ignored the typescript errors, but I figured I spare the codebase a bunch of // @ts-ignore's.
  3. Added a typescript-compatible linter
  4. Added a build-script for compiling CJS, ESM, and types
  5. Modified the package.json to distribute the correct builds based on the installers project

A few things I didn't touch:

  1. I didn't modify any of the testing logic, I allowed eslint to format the file though
  2. I didn't modify any of the fixtures. After converting the code to typescript, all of the tests ran perfectly. Why fix what's not broken lol

@pirxpilot
Copy link
Owner

Thanks - but I'd rather not.

This is a server-only module. I will rewrite it as ESM at some point but I do not really see any reason to increasing bundle size to have both formats exported (since one can import common JS module in ESM code). I will change a linter at some point but probably to something modern and not eslint.

I admit those are personal preferences. Goes without saying you are most welcome to use your fork.

@pirxpilot pirxpilot closed this Sep 28, 2024
@alexfigliolia
Copy link
Author

Smart to close, there required an entry-point extension fix required to properly release. If you change your mind I'll reopen

@alexfigliolia
Copy link
Author

(PSA)

On the topic of ESM/CJS, projects do exist where once you convert to ESM, CJS projects will break and be locked into a lower major version of your package. It's unfortunate, but does exist see Chalk's decision to support common.js then ESM. All CJS projects using TypeScript broke (this was thousands of open source/production repos) and were locked on a major version below the latest release for about 3 years.

@pirxpilot
Copy link
Owner

Yup. That's why I'd wait for Node 24 LTS supporting sync ESM require without experimental flag.

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.

2 participants