lib: include ESM loader in the built-in snapshot#61769
lib: include ESM loader in the built-in snapshot#61769joyeecheung wants to merge 6 commits intonodejs:mainfrom
Conversation
|
Review requested:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #61769 +/- ##
==========================================
+ Coverage 89.74% 89.75% +0.01%
==========================================
Files 675 674 -1
Lines 204735 204750 +15
Branches 39348 39341 -7
==========================================
+ Hits 183737 183781 +44
+ Misses 13269 13254 -15
+ Partials 7729 7715 -14
🚀 New features to boost your workflow:
|
watilde
left a comment
There was a problem hiding this comment.
One question for my understanding: I noticed defaultResolve and defaultLoadSync are still lazy-loaded with ??= while translators was moved to pre-loading. Is this because:
- These are only used when there are no custom loader hooks (less common path)?
- Their dependencies are heavier and would hurt startup more?
- Or is this something that could be optimized similarly in the future?
Just curious about the design rationale
Did you mean in |
|
My code base in my local was very old, nvm. Thank you! |
Commit Queue failed- Loading data for nodejs/node/pull/61769 ✔ Done loading data for nodejs/node/pull/61769 ----------------------------------- PR info ------------------------------------ Title lib: include ESM loader in the built-in snapshot (#61769) Author Joyee Cheung <joyeec9h3@gmail.com> (@joyeecheung) Branch joyeecheung:esm-in-snapshot -> nodejs:main Labels lib / src, needs-ci, commit-queue-rebase Commits 3 - lib: remove top-level getOptionValue() calls in lib/internal/modules - lib: reduce cycles in esm loader and load it in snapshot - benchmark: add startup benchmark for ESM entrypoint Committers 1 - Joyee Cheung <joyeec9h3@gmail.com> PR-URL: https://github.com/nodejs/node/pull/61769 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/61769 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> -------------------------------------------------------------------------------- ℹ This PR was created on Wed, 11 Feb 2026 04:52:49 GMT ✔ Approvals: 3 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/61769#pullrequestreview-3782914586 ✔ - Daijiro Wachi (@watilde): https://github.com/nodejs/node/pull/61769#pullrequestreview-3784562017 ✔ - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/61769#pullrequestreview-3787902819 ✘ Last GitHub CI failed ℹ Last Full PR CI on 2026-02-12T15:02:50Z: https://ci.nodejs.org/job/node-test-pull-request/71322/ - Querying data for job/node-test-pull-request/71322/ ✔ Build data downloaded - Querying failures of job/node-test-commit/85439/ ✔ Data downloaded ✘ 44 failure(s) on the last Jenkins CI run -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/21975249143 |
71c535a to
9cd87a2
Compare
|
Looks like this got broken by #61665 which added filtering of internal frames but the regex couldn't understand async frames. Updated the regex to handle async frames. cc @legendecas |
|
Looks like the new regex didn't work with test runner outputs somehow. Updated. Can you take a look again? @legendecas Thanks! |
Co-authored-by: Chengzhong Wu <legendecas@gmail.com>
There was a problem hiding this comment.
Is node:sqlite omitted intentionally from here?
There was a problem hiding this comment.
It's copied from the require-builtins which predated sqlite. Can update both in a follow up (I think there are probably a few other newer builtins not included too).
There was a problem hiding this comment.
I'm wondering if there could be a new lint rule just to keep these 2 files up to date at all times? I'm not knowledgeable enough in ESLint to implement this, but maybe it is a viable path to ensure these files do not get left behind as builtins change (added, removed, renamed) over time.
There was a problem hiding this comment.
IMO a lint rule is a bit of an overkill - that kind of put the pressure on anyone who adds a new built-in to run the benchmark and measure its loading performance when that's not really a universally important metrics for all builtins. For builtins that are changed or removed we have already smoking benchmark tests to ensure these benchmarks don't throw.
Benchmark number from an ARM64 Linux machine: empty/minimal CJS startup is now slightly slower in worker but other metrics get a slight boost (because they all incur ESM loader initialization). In reality ESM loading is likely to happen at some point in the lifetime of an application especially with the growing adoption of ESM and
require(esm), so real-world applications that are more than just an empty script should get a bit of speed up from being able to just deserialize the ESM loader instead of initializing it from scrach.