cjs-module-lexer icon indicating copy to clipboard operation
cjs-module-lexer copied to clipboard

Allow externalized Wasm build

Open guybedford opened this issue 1 year ago • 1 comments

We landed and reverted https://github.com/nodejs/cjs-module-lexer/pull/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

guybedford avatar Apr 28 '24 17:04 guybedford

Also see the explanation in https://github.com/nodejs/cjs-module-lexer/pull/94#issuecomment-2081593370.

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 avatar Apr 30 '24 14:04 mochaaP