-
Notifications
You must be signed in to change notification settings - Fork 25
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
Support building for externally shared js builtins #91
Support building for externally shared js builtins #91
Conversation
@mhdawson This is closely related to my/our work on distro-level WASM blob rebuilds. Could you take a look at this? |
cc @guybedford request for review :) |
@khardix thanks for the heads up, will take a look tomorrow. |
@mochaaP I took a look through and could you confirm my understanding that the following:
<EXTERNAL_PATH>/lexer.js
/dist/lexer.wasm Node.js will use <EXTERNAL_PATH>/lexer.js as the builtin for dist/lexar and then read the wasm blob from <EXTERNAL_PATH>/dist/lexer.wasm Overall I think that makes sense to me except that it seems to be that lexer.js and lexer.wasm should both be at the same level instead of one extra dist for lexer.wasm. I'd have to double check but I think when specifying --shared-builtin-cjs_module_lexer/dist/lexer-path= that should point to a directory with the contents of dist/lexer.js. If the expectation was for <EXTERNAL_PATH>/lexer.js
/lexer.wasm and the build step was also updated to copy lib/lexer.wasm to dist/lexer.wasm when EXTERNAL_PATH was specified, then providing package that installs the contents of the dist after the build to EXTERNAL_PATH would be more obvious. Futher I assume that in most cases people will want to use both: In that context it might make sense to change where files get written when EXTERNAL_PATH is defined so that a directory externalized-dist/lexer.js
/wasm-lexer/lexer.js
/wasm-lexer/lexer.mjs
/wasm-lexer/lexer.wasm And then the contents of externalize-dist is bundled into the package to be built/distributed separately. If that makes sense the possibly copying over the licence and maybe the readme into externalized-dist might make sense as well. The current case where Node.js uses files from the top level as well as dist seems a bit odd to me, but would breaking to fix, but since use of the externalized build will be new it offers the opportunity to tweak a bit. I think in this case when building to externalize the lexer, Node.js could then set Also note this is based on how I remeber these options working in terms of what you provide as a path, but it was a while back when I added the option so not 100% sure without some testing. Just some thoughts, will need @guybedford to comment on wether any of these suggestions make any sense. |
See https://github.com/nodejs/node/blob/9a25438a62186fa5bd2603536d99dcccd0acd893/lib/internal/modules/esm/translators.js#L82-L93. Loading builtins will need an exact match for the import specifier, so importing from a subdirectory is non-applicable, since there is no filesystem present.
A problem with the |
Initial support for loading unbundled module in `AddExternalizedBuiltin`. - Reduces downstream distribution package size (by not shipping wasm twice and not base64-encoding it) - Provides a cleaner stacktrace - Easier to patch To enable this, pass `EXTERNAL_PATH=/global/node_modules/cjs-module-lexer` to `build.js`. You shall also pass this path to `--shared-builtin-cjs_module_lexer/dist/lexer-path` in Node.js's `configure.py`, with the extra `/dist` part in the path. Signed-off-by: Zephyr Lykos <git@mochaa.ws>
afc8eae
to
3c5407e
Compare
Converted indentation in |
My point is that using something outside of the dist directory seems odd since if dist is meant to contain externally used assets, also using files outside of dist is non-intuative, at least to me. |
If the approach makes sense to @guybedford then I think adding externalized-dist to the "files" section in the package.json should be acceptable. Depending on how the package was built, dist or externalized dist would be empty but I doubt the extra space of a empty directory would be objectionable. I think the main thing is if the concept of everything being used externally being a single directly or not makes sense/is intended. I look to @guybedford to comment on that. |
I think it's better to keep the current directory structure in case any third-party package depends on the |
@mochaaP what I was suggesting would only change things if the package used the EXTERNAL_PATH option, would third party packages be able to find the files in the Fedora delivered install? |
I think that still breaks things, especially when there weren't any obvious outcomes 🧐 |
Thanks for creating the PR, can't we just make the external path the default build approach instead of bifurcating the entire build process? The base64 technique comes from es-module-shims which runs in the browser primarily hence the benefits of a single file, but since cjs-module-lexer is mostly for Node.js usage we could just use this technique by default and have a single file that reads a local Wasm file. I'd prefer if we could not separate the |
When importing this in Node.js itself, how is the extra Wasm file loaded from a path inside of Node.js though? |
Node.js loads builtins as a single script file under the However, if us (downstream distribution) decided to unbundle the module, we could get the benefit of faster startups and smaller update sizes due to reduced
When importing the module from
This is handled by esbuild (see the |
Ideally the best way we could achieve these same goals would be by fully migrating this project as a C++ build, work which was already outlined in https://github.com/anonrig/commonjs-lexer. There would be performance benefits to be had as well that way. Otherwise it seems this PR doesn't apply to anything other than a very custom wrapping approach - my major concern there is that even if we land this, we aren't necessarily testing that embedding itself so it seems a little fragile. I would be happy to land the PR as-is if @mhdawson also approves. Just talking through the wider concerns, given that I can't comment that well on the particular embedding. |
I'll chat with @khardix on Friday, if he's good with the PR as is, I'm ok as well. I still think the structure could be better, but not enough to block progress on this. |
Talked to @khardix , looks like its good from his perspective. He also mentioned that something similar went into undici nodejs/undici#2643 so it is good to be consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@guybedford review request :) |
Hm, and since this is approved, can anyone push the merge button? 😇 I see the failed CI, but GH no longer show me any details as to why. |
iirc it's a github actions outage |
@guybedford could you take a look at this? |
There does not seem to be any easy way to restart the action for the test. |
f678b6c
to
e17dd9d
Compare
@mhdawson The workflow requires approval from maintainer to run. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM again
Oh I think the culprit here is that ubuntu-18.04 is no longer available in hosted runners. Consider migrate to |
e742a8d
to
dedfe17
Compare
dedfe17
to
7ca131a
Compare
CI succeeded in my repo: https://github.com/mcha-forks/cjs-module-lexer/actions/runs/8661836362 |
since the workflow has changed drastically, re-requesting reviews. |
@mochaaP were all the workflow changes needed because of the move to unbuntu-latest ? Needs a review from @guybedford, since he's the primary maintainer for this repo. |
nope, but I think it's better done in this way :) |
also |
Thanks! I would be grateful if you could publish a new release, so we don't have to rely on snapshot versions / patch it ourselves |
@mochaaP 1.3.0 has been released with this change. |
cc @guybedford I need a quick patch here: afcf9f1ce4a10c0l62411e9d9e106b5c0072ef783 i somehow missed an |
This reverts commit 1c5072c.
Initial support for loading unbundled module in
AddExternalizedBuiltin
.Refactored to use esbuild under the hood.
To enable this, pass
EXTERNAL_PATH=/global/node_modules/cjs-module-lexer
tobuild.js
.You shall also pass this path to
--shared-builtin-cjs_module_lexer/dist/lexer-path
in Node.js'sconfigure.py
, with the extra/dist
part in the path.Reference:
nodejs/node@ca5be26b318
nodejs/node#44376
https://src.fedoraproject.org/rpms/nodejs-cjs-module-lexer/pull-request/2
nodejs/undici#2643