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 build for browsers #342

Merged
merged 2 commits into from
Oct 18, 2020
Merged

Add ESM build for browsers #342

merged 2 commits into from
Oct 18, 2020

Conversation

calebeby
Copy link
Contributor

Hi @evanw!

I added a ESM build for use in browsers, so that the CJS shim code is not needed for browsers which support ESM natively. I added it to pkg.module which is enough for my use case.

As a bonus, I added pkg.exports, which is not necessary for my use case, but I thought I might as well. I saw the discussion in this issue and since then, the discussions between webpack and rollup authors (and others) seem to have stabilized. Here's a good summary of those discussions (keep in mind they went with module instead of moduleOnly). If you decide that you aren't quite ready to have pkg.exports here, I am totally fine with removing that from this PR 🙂

Thank you for your work with esbuild! I am very excited to see where this project goes in the future, and I am looking forward to new competition in the tooling landscape!

@@ -87,4 +87,4 @@ let api: typeof types = {
startService,
};

module.exports = api;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was needed, because without it, ESM shim code was being included in the ESM build. I switched it so that now it uses a default export as well as named exports. I took a look at the outputs of the various bundles and it seems correct. The bundle that I'm least sure about is the browser.js bundle, but it looks right as far as I can tell.

"exports": {
".": {
"browser": {
"module": "./lib/browser.mjs",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ./lib/browser.mjs does not import any CJS files, so it qualifies for a "module-only optimization" as described here. I think it is not necessary to additionally have the import field, since bundlers will prefer module over import, and only node needs import, but node does not look at browser.

const browserMjs = childProcess.execFileSync(esbuildPath, [
path.join(repoDir, 'lib', 'browser.ts'),
'--bundle',
'--target=es2020',
Copy link
Contributor Author

@calebeby calebeby Aug 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set it to a modern target since the assumption for the .mjs build is that if people need it to work in older browsers, they can transpile it themselves.

@calebeby
Copy link
Contributor Author

calebeby commented Oct 4, 2020

Hey @evanw Could you take a look at this?

@evanw
Copy link
Owner

evanw commented Oct 18, 2020

Thanks for the ping. I can definitely ship something like this.

The export logic isn't quite right though since you don't want bundlers that support modules picking up browser.mjs for non-browser targets, so I'm going to change how exports are configured before shipping it.

@evanw evanw merged commit cfaedae into evanw:master Oct 18, 2020
@evanw
Copy link
Owner

evanw commented Oct 18, 2020

Changes I made:

  • I tested both builds in the browser and fixed everything that caused it to be broken (e.g. no ESBUILD_VERSION).

  • I changed the target es2020 to es2017 because otherwise the use of certain newer features such as optional catch bindings would unnecessarily break in older browsers that still support esm syntax.

  • I removed exports in package.json because as far as I know, adding it is a backwards-incompatible change, and I don't want this to break backwards compatibility.

  • I removed module because the esbuild-wasm package can also be used in node, and bundling it with a module-aware bundler shouldn't cause the browser build to be bundled.

    Technically the way to do this is to keep the module field but make it a node esm build, then add a browser mapping that overrides both main and module. However, the node code relies on __dirname for which the replacement is import.meta.url which is new and not supported everywhere, so I backed out of that change.

  • I moved the lib/browser.mjs file to the esm folder instead to avoid potential issues due to adjacent files with the same name but different extensions.

  • I renamed the file to esm/browser.js because some web servers serve .mjs files with application/octet-stream which causes the browser to refuse to parse the JavaScript code.

  • I removed the unnecessary default export.

So basically the only change in the next release will be a new esm/browser.js file, without any changes to package.json at the moment.

evanw added a commit that referenced this pull request Oct 18, 2020
@calebeby calebeby deleted the wasm-esm branch October 19, 2020 23:54
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