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

Allow externalized Wasm build #96

Open
guybedford opened this issue Apr 28, 2024 · 1 comment
Open

Allow externalized Wasm build #96

guybedford opened this issue Apr 28, 2024 · 1 comment

Comments

@guybedford
Copy link
Collaborator

guybedford commented Apr 28, 2024

We landed and reverted #91 which gave the ability to point to an externalized Wasm file for the lexer build.

As far as I'm aware the primary requirement here was not to use base64 encoding, but to instead load the Wasm from a separate file, that could be vetted at distribution time.

Therefore the suggestion is to have two builds of cjs-module-lexer - one like the existing one that inlines the base64 and one new one that automatically loads the lexer wasm from a separate wasm file.

In our build process we then generate these two new builds allowing consumers to select which one they want to use.

There is therefore:

  • No need for a runtime environment variable at all
  • We can ALWAYS generate two builds - one with the inlined binary, and one that loads the binary from a separate file
  • No environment variable is then needed for the build script at all
  • Only one define variable is needed for WASM_BINARY- whether or not the binary is being generated inline, with the undefined case indicating the external case, unless we absolutely need to customize the external path at build time?

I am also still unclear on why a new exports entry point was needed for this alternative build as well. There is already CJS entry point generation in the npm run build script, which just needs to be extended to the new entry point being generated as well.

//cc @mochaaP

@mochaaP
Copy link
Contributor

mochaaP commented Apr 30, 2024

Also see the explanation in #94 (comment).

No need for a runtime environment variable at all

There was never one.

We can ALWAYS generate two builds - one with the inlined binary, and one that loads the binary from a separate file

Sounds good to me!
Does this mean that with upstream Node builds, we use the inline binary ones (CJS, not exported in package.json), and load from an external file when loaded as a NPM module (ESM, exported in package.json)?

If this is the case: we'd like to keep a separate script in this repo, to generate a CJS shim for Node to load the builtin from an external NPM module in downstream distro builds.

Also, can I implement this by further stripping on top of #91? To be honest, I'm not a huge fan of @babel/cli and terser here 😅. CI was also broken by the revert.

No environment variable is then needed for the build script at all

An addition optional --without-cjs/esm flag could be nice, but that's not a hard requirement. Please let me know what you think about it.

Only one define variable is needed for WASM_BINARY- whether or not the binary is being generated inline, with the undefined case indicating the external case, unless we absolutely need to customize the external path at build time?

Yes. That would work!

mochaaP added a commit to mcha-forks/cjs-module-lexer that referenced this issue Apr 30, 2024
mochaaP added a commit to mcha-forks/cjs-module-lexer that referenced this issue Apr 30, 2024
mochaaP added a commit to mcha-forks/cjs-module-lexer that referenced this issue Apr 30, 2024
mochaaP added a commit to mcha-forks/cjs-module-lexer that referenced this issue May 7, 2024
mochaaP added a commit to mcha-forks/cjs-module-lexer that referenced this issue May 7, 2024
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

No branches or pull requests

2 participants