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

build(package): generate esm output with exports #3337

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kwonoj
Copy link

@kwonoj kwonoj commented Sep 6, 2023

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #3335

This PR attempts to address #3335. Given there are a lot of possible ways to achieve cjs/esm compatible pkg support, I can see this PR is not feasible to this repo's convention and likely can be closed without merge. That's totally fine, however would like to kickstart an initial effort.

Among various approach, this PR nearly mimics recent redux-toolkit's approach (without bundling part) and my personal experiences when I dealt with RxJS (though it's not real esm - https://github.com/ReactiveX/rxjs/blob/master/package.json). Detailed exploration can be found at https://blog.isquaredsoftware.com/2023/08/esm-modernization-lessons/, but in crux this PR does these things

  • Preserve package.json's type, so existing cjs-based scripts & assumptions (require..) works as-is
  • Adjust existing cjs output to build/cjs
  • Introduce another tsconfig to generate esm outputs, generates under build/esm. This uses tsc-multi(https://github.com/tommy351/tsc-multi), which is a wrapper to generate multiple target outputs with manually transforming extensions per each.
    • Using above tsc-multi, make build/esm uses .mjs extension to let node.js resolves this as esm explicitly
  • Adjust generators template to include .js extension by default
  • Apply conditional exports to package.json for import / require and lastly let default resolves to esm.

Quick local testing confirmed generated pkg can be imported in node.js with cjs / esm (type:module), also typescript definitions are being resolved. The caveat for type resolution is it falls back to cjs types always, but it won't be a huge concern since most of types should be equivalent between 2 build outputs. Below's manual testing from https://arethetypeswrong.github.io/ how imports are being resolved.

┌───────────────────┬───────────────────────────┬────────────────────────┐
│                   │ "googleapis/package.json" │ "googleapis"           │
├───────────────────┼───────────────────────────┼────────────────────────┤
│ node10            │ 🟢 (JSON)                 │ 🟢                     │
├───────────────────┼───────────────────────────┼────────────────────────┤
│ node16 (from CJS) │ 🟢 (JSON)                 │ 🟢 (CJS)               │
├───────────────────┼───────────────────────────┼────────────────────────┤
│ node16 (from ESM) │ 🟢 (JSON)                 │ 🎭 Masquerading as CJS │
├───────────────────┼───────────────────────────┼────────────────────────┤
│ bundler           │ 🟢 (JSON)                 │ 🟢                     │
└───────────────────┴───────────────────────────┴────────────────────────┘

One last thing to note is about conditional exports: conditional exports limits arbitaray imports (including cjs require) to subpath imports, so if anyone used subpath imports like require('googleapi/somepath/imports') explicitly it'll likely fail. I'm not certain this should be treated as major breaking - since the entrypoints reexports everything and individual api should be resolvable withput subpath - though.

@kwonoj kwonoj requested a review from a team as a code owner September 6, 2023 04:30
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Sep 6, 2023
@kwonoj
Copy link
Author

kwonoj commented Sep 6, 2023

I did use these to test resolution locally:

//cjs.cjs

const x = require('googleapis');

console.log(require.resolve('googleapis'));
//esm.mjs
// run with node --experimental-import-meta-resolve esm.mjs

(async () => {
  const googleapi = await import.meta.resolve('googleapis');

  console.log(googleapi)
})();

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide an ESM output target
2 participants