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

Add ESM #400

Closed
wants to merge 2 commits into from
Closed

Add ESM #400

wants to merge 2 commits into from

Conversation

loynoir
Copy link

@loynoir loynoir commented Apr 21, 2023

Main Goal

decrease bundle size #399

Sub Goal

Impossible Goal

  • Configure tsconfig to enforce coding style must explicit import extension.
-import { Assert } from '../assert'
+import { Assert } from '../assert/index.js'

Changes

  • To achive 100% backward compact with CJS, and using ESM, now it is dual package using conditional exports.

  • Add explicit import extension, as ESM runtime needs explicit import extension.

Before

$ npm exec -- esbuild --format=esm --bundle --outfile=reproduce.output.mjs reproduce.mjs
  reproduce.output.mjs  148.7kb

After

$ cp /path/to/fork/typebox/target/build/sinclair-typebox-0.28.1.tgz build/
$ npm remove @sinclair/typebox
$ npm i ./build/sinclair-typebox-0.28.1.tgz
$ npm exec -- esbuild --format=esm --bundle --outfile=reproduce.output.mjs reproduce.mjs

  reproduce.output.mjs  129.8kb

@loynoir loynoir force-pushed the feat-esm branch 3 times, most recently from b2b8029 to a310499 Compare April 21, 2023 05:07
@sinclairzx81
Copy link
Owner

sinclairzx81 commented Apr 21, 2023

@loynoir Hi, thanks for this PR, I'll need some time to test and review it. Just be mindful that it's very important not to break existing installations across each platform (so Node, Deno (both NPM and here), Bun, and esm.sh)

Do you have any reference links on best practice to support both ESM and CJS? I assume the cjs and esm submodule pathing is common, but curious on best practice configuration of package.json. I had been meaning to support ESM, however probably as a minor semver, as internal directories have been changed.

@loynoir
Copy link
Author

loynoir commented Apr 21, 2023

#!/usr/bin/node --experimental-import-meta-resolve
// @ts-check
import { createRequire } from 'node:module'
const require = createRequire(import.meta.url)

console.log({
  requireResolve: require.resolve('@sinclair/typebox'),
  importMetaResolve: await import.meta.resolve('@sinclair/typebox')
})

Runtime resolve is correct, tested under node v18.15.0

{
  requireResolve: '/path/to/node_modules/@sinclair/typebox/cjs/typebox.js',
  importMetaResolve: 'file:///path/to/node_modules/@sinclair/typebox/esm/typebox.js'
}

I don't know if there is official guide.

@loynoir
Copy link
Author

loynoir commented Apr 21, 2023

Wait a minute.

await import('@sinclair/typebox')

Editor dts resolve to node_modules/@sinclair/typebox/cjs/typebox.d.ts, maybe a bug. I'm going to open a typescript issue.

@loynoir
Copy link
Author

loynoir commented Apr 21, 2023

@sinclairzx81

microsoft/TypeScript#52593

microsoft/TypeScript#53045 (comment)

await import('@sinclair/typebox')

Editor dts resolve to node_modules/@sinclair/typebox/cjs/typebox.d.ts.

Yet, seems a typescript bug to me.

Nothing can be done here.

But, the good news is they are same.

So, even dts resolve wrong, it is correct. 😉

$ md5sum node_modules/@sinclair/typebox/{cjs,esm}/typebox.d.ts
e7d858fbfbb889c8739cb8de1f5601bb  node_modules/@sinclair/typebox/cjs/typebox.d.ts
e7d858fbfbb889c8739cb8de1f5601bb  node_modules/@sinclair/typebox/esm/typebox.d.ts

@sinclairzx81
Copy link
Owner

@loynoir Thanks for doing this research into this.

I think at this stage, I may need to hold off merging this PR till the next minor semver (and probably publish on a -dev tag to NPM such that downstream implementers can test everything is ok). Comments from Ryan here and here suggest the TS team are working on some formal guidelines for dual module publishing to NPM (which I assume would be inclusive TS module resolve (build time) as well as guidelines for module CDN bundlers (like esm.sh), but the second link (which suggests there may be a requirement to transform the code during the build process) is something I'm keen to avoid if possible.

Would you be willing to defer this PR till the next minor semver? (possibly 2-4 weeks) ?

@loynoir
Copy link
Author

loynoir commented Apr 21, 2023

@sinclairzx81

I'm fine with that.

In case of at that time point, I am busy.

All noticeable informations are

  1. Currently, seems no way to let tsconfig to enforce import extension.

  2. As result of 1: After merge this pr, instead of write code import ... from 'foo/bar', you should write code import ... from 'foo/bar.js' or import ... from 'foo/bar/index.js'.

  3. Current mocha test bundle with typescript source, instead of bundle with typescript output source. (I am lazy to change to that.)

  4. As result of 2 and 3: There is no test to enforce you to explicit import extension. If you forget to explicit import extension, ESM client will got runtime error which not detected in test.

  5. esm entry dts resolve to cjs entry dts, if a user use javascript with "checkJs": true instead of typescript. dts resolve to CJS entry dts instead of ESM entry dts when not included in tsconfig microsoft/TypeScript#53958

@sinclairzx81
Copy link
Owner

@loynoir Hi, just following up on this PR.

I've been through the changes, and I don't think I can merge this through at this time. These changes are too far reaching (338 files changed) and there's still a lot of uncertainty around dual publishing CJS and ESM in general (with the uncertainly around dual publishing making such a large change on the codebase untenable in the instance better approaches eventuate down the road). There are some recommendation guidelines in the works (as linked microsoft/TypeScript#52593) which I'm going to let settle for a while, but I think is the best call for now is to wait and see.

I think rather than explicit appending each import with the .js specifier, I think I may take a look at automating this process via a build script (automating from CJS to ESM + specifier). The reason to go about things this way is it would involve no changes to the current codebase, while allowing build tools to transform for Deno (which uses .ts import specifiers), Node (which could use .mjs or .js specifiers) as well as provide flexibility for other specifiers should they be required in future (inclusive of protocol prefixing should that be more widely adopted in future). I've got similar build workflows for some other projects I manage, and may look at adding some built in tools to hammer which would be based on formal guidelines for dual publishing. I think this is generally the more low impact solution to ESM overall.

As for module splitting (file size optimization), this is very much under consideration, but won't be carried out for another few minor semver iterations. There has been some desire to offer better modularity / splitting of the codebase in places (outside of this PR and associated issues), so will be taking a look at this eventually, but for now there's a necessity to stabilize the previous 3 minor semver revisions before carrying out this work, and it's going to be difficult to carry this PR along in it's current form until such time.

Apologies for taking so long to get back to you on this PR. I appreciate this work and motivation behind it. But will need to revisit at a later time. I hope you understand. Will close off and revisit down the road.
Many Thanks
S

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