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

🏗 Use ESM to import peer Bento binaries #37586

Merged
merged 27 commits into from
Feb 11, 2022

Conversation

alanorozco
Copy link
Member

@alanorozco alanorozco commented Feb 6, 2022

On module builds, use browser's import statement to share Bento dependencies like bento.mjs.

On nomodule builds, we can't use import. Instead, we use babel to output to a format similar to AMD.

This change also simplifies some aspects:

  • Prefers using {remapDependencies} for esbuild rather than module-resolver on babel.
  • Removes bento compile wrapper.
  • Removes generators. Uses src/bento.js directly, and the intermediate bento-shared.js module is no longer needed.

build-system/tasks/helpers.js Outdated Show resolved Hide resolved
build-system/compile/bento-remap.js Outdated Show resolved Hide resolved
build-system/tasks/helpers.js Outdated Show resolved Hide resolved
@alanorozco alanorozco marked this pull request as ready for review February 9, 2022 20:18
@amp-owners-bot
Copy link

amp-owners-bot bot commented Feb 9, 2022

Hey @erwinmombay, @jridgewell! These files were changed:

build-system/babel-plugins/babel-plugin-nomodule-loader/define-template.js
build-system/babel-plugins/babel-plugin-nomodule-loader/index.js
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-default/input.mjs
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-default/options.json
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-default/output.js
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-from/input.mjs
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-from/options.json
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/export-from/output.js
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/import-default/input.mjs
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/import-default/options.json
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/import-default/output.js
build-system/babel-plugins/babel-plugin-nomodule-loader/test/fixtures/transform/imports-and-exports/input.mjs
+12 more

Hey @rsimha! These files were changed:

build-system/common/esbuild-babel.js
build-system/common/once.js

@alanorozco
Copy link
Member Author

alanorozco commented Feb 9, 2022

@jridgewell this should be ready for review. PTAL

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Can you add a test case for a single exports default fn() {} export? Want to make sure isn't not going to do a module.exports = fn() {}.

Otherwise, LGTM.

alanorozco and others added 15 commits February 11, 2022 08:54
I don't know why I didn't do this originally, it's sooo much simpler. This has babel output an inline sourcemap, which is fed into esbuild. Esbuild will do an initial sourcemap-aware bundling, and output a new map that correctly points through babel into our real source files.

After that, we can do the simple define/minify remapping for our post compile.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants