esm: avoid import.meta setup costs for unused properties

[JakobJingleheimer]

Currently, the cost of deriving import.meta.dirname and import.meta.filename is paid up-front, regardless of whether they are actually used. This can instead be delayed until they are accessed, and then to avoid re-paying the cost, overwrite the getter with the derived value.

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[jsumners-nr]

For what it's worth, my personal time identity backed this out due to a lot of waffling and someone finally providing evidence against it -- #48740 (comment)

(Very easy to overlook that in all of the nonsense on that PR.)

[codecov]

Codecov Report

Attention: Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.27%. Comparing base (3186468) to head (c3c12a4).
Report is 546 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/modules/esm/initialize_import_meta.js	93.93%	2 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #57003 +/- ##
==========================================
+ Coverage 89.13% 90.27% +1.14% 
==========================================
Files 665 630 -35 Lines 193125 186189 -6936 Branches 37182 36474 -708 ==========================================
- Hits 172136 168089 -4047 + Misses 13734 10979 -2755 + Partials 7255 7121 -134 

Files with missing lines	Coverage Δ
lib/internal/modules/esm/initialize_import_meta.js	98.16% <93.93%> (-1.84%)	⬇️

... and 270 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JakobJingleheimer]

(Very easy to overlook that in all of the nonsense on that PR.)

😆

For what it's worth, my personal time identity backed this out due to a lot of waffling and someone finally providing evidence against it -- #48740 (comment)

It looks like the spec was misquoted.

I'm not sure what Antoine was getting at with

I don't get it though, the getters are evaluated right away, there's no point in keeping them or am I missing something?

Getters are not run right away—that's their whole point.

[jsumners-nr]

I don't get it though, the getters are evaluated right away, there's no point in keeping them or am I missing something?

Getters are not run right away—that's their whole point.

They were in that specific implementation. But I was quite happy to stop churning on the detail.

I am not against these being getters. I'm just linking the history.

[ljharb]

https://tc39.es/ecma262/#sec-meta-properties-runtime-semantics-evaluation

The spec’s intention is very much that import.meta only contains data properties. It’s surely possible to use C++ to make an object that appears to have data properties but avoids paying the up front cost, but it wouldnt be possible in JS.

As is, this change is observable to someone looking at property descriptors, and this i think needs to be semver major.

[hybrist]

https://tc39.es/ecma262/#sec-meta-properties-runtime-semantics-evaluation

The spec’s intention is very much that import.meta only contains data properties. It’s surely possible to use C++ to make an object that appears to have data properties but avoids paying the up front cost, but it wouldnt be possible in JS.

As is, this change is observable to someone looking at property descriptors, and this i think needs to be semver major.

I agree with Bradley in the other thread that HostFinalizeImportMeta does exist in that algorithm and gives the host/nodejs explicit permission to make other changes to the object, including adding non-data properties.

I don't think the exact property descriptor is "part of the API" here but I don't feel strongly enough about it to say that it must be semver minor over major.

[ljharb]

It absolutely allows it - but the intention was for freezing and not for making getters, as i recall.

[mcollina]

lgtm

[bmeck]

@ljharb as I recall it was for freezing from Agoric side of convo and altering proto from web vendor side (including a potential desire for accessors from proto).

[ljharb]

ah, I don't recall that second part.

[bmeck]

@ljharb they never shipped it and likely never will at this point

[bmeck]

an alternative that is still a data property and is lazy is to use https://v8.github.io/api/head/classv8_1_1Object.html#a918a985db244fdc1bde268741e840620

[ljharb]

I still consider this a breaking change, but LGTM

[JakobJingleheimer]

Ah, damn. Looks like it will miss 24.0.0; I was thinking it still might land for that and breaking/non-breaking would be a discussion we don't have to have 😅 (aside from backport).

[ljharb]

Doing it in C++ would make it nonbreaking, if that's preferred.

[JakobJingleheimer]

Doing it in C++ would make it nonbreaking, if that's preferred.

Mmm, I was hoping/expecting that approach to tick all the boxes. But it seems a mixed bag, IMO leaning towards the less ideal.

I was just planning to get this PR to a landable state so then we can decide A or B.

To me, the approach in this PR is in the vicinity of breaking territory in that it may be observable. But, I'm thinking it won't actually be tangible (and thus won't actually break anything).

[ljharb]

I agree that breakage is unlikely, but semver doesn't deal in likelihoods :-)

fwiw it seems perfectly fine to land this on 25+, and then convert it to C++ in a future PR, at which point both commits would be maximally backportable together.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66416/

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/57003
✔ Done loading data for nodejs/node/pull/57003
----------------------------------- PR info ------------------------------------
Title esm: avoid `import.meta` setup costs for unused properties (#57003)
⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch JakobJingleheimer:esm/avoid-setup-costs-for-unused-importmeta-props -> nodejs:main
Labels semver-major, esm, author ready, needs-ci, commit-queue-squash
Commits 3
- esm: avoid `import.meta` setup costs for unused properties
- !fixup: correct autocomplete of prop name
- fixup!: add test case for re-assigning `dirname` & `filename`
Committers 1
- Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com>
PR-URL: https://github.com/nodejs/node/pull/57003
Reviewed-By: Jan Martin <jan.krems@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jordan Harband <ljharb@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/57003
Reviewed-By: Jan Martin <jan.krems@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Jordan Harband <ljharb@gmail.com>
--------------------------------------------------------------------------------
ℹ This PR was created on Tue, 11 Feb 2025 10:54:37 GMT
✔ Approvals: 3
✔ - Jan Martin (@jkrems): https://github.com/nodejs/node/pull/57003#pullrequestreview-2785333954
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/57003#pullrequestreview-2610219287
✔ - Jordan Harband (@ljharb): https://github.com/nodejs/node/pull/57003#pullrequestreview-2785323386
✘ semver-major requires at least 2 TSC approvals
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2025-04-22T20:58:45Z: https://ci.nodejs.org/job/node-test-pull-request/66416/
- Querying data for job/node-test-pull-request/66416/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/14614621512

[JakobJingleheimer]

Ah, needs 2 TSC approvals, not just 1.

[aduh95]

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1694/

esm/esm-legacyMainResolve.js resolvedFile='node_modules/test/index.js' packageConfigMain='' packageJsonUrl='node_modules/test/package.json' n=10000 ** -0.80 % ±0.52%
esm/import-meta.js n=1000 *** -9.01 % ±0.81%

[JakobJingleheimer]

I don't know how to read that benchmark 😅

Could we also run CitGM? (I don't know how to trigger it)

[aduh95]

The benchmark shows a worst result than #57286 for esm/import-meta.js, I guess that's not surprising, and is not a deal breaker I don't think. That being said, it'd probably be a good idea for me (or someone else) to go back to that other PR to see if where we could improve it as it would address Jordan concerns and be potentially better perf-wise

[aduh95]

Benchmark CI to include the results form #58056: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1710/

 confidence improvement accuracy (*)
esm/esm-loader-import.js specifier='node:prefixed-nonexistent' n=1000 * -3.67 % ±3.56%
esm/import-meta.js valuesToRead='dirname-and-filename' n=1000 *** -7.97 % ±0.62%
esm/import-meta.js valuesToRead='dirname' n=1000 *** -7.81 % ±0.82%
esm/import-meta.js valuesToRead='filename' n=1000 *** -6.14 % ±0.83%

[aduh95]

Superseded by #57286