[v20.x] backport unflagging of require(esm), part 1 (of 4?)#56730
Uh oh!
There was an error while loading. Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
PR-URL: nodejs#54619 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55255 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Marco Ippolito <[email protected]>
Fixes: nodejs#55208 PR-URL: nodejs#55249 Reviewed-By: Yagiz Nizipli <[email protected]>
The actual implementation returns `outgoingMessage` itself, but not exactly `http.ServerResponse`. Refs: https://github.com/nodejs/node/blob/20d8b85d3493bec944de541a896e0165dd356345/lib/_http_outgoing.js#L712-L751 PR-URL: nodejs#55290 Reviewed-By: Paolo Insogna <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Qingyu Deng <[email protected]>
PR-URL: nodejs#55334 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Bumps [step-security/harden-runner](https://github.com/step-security/harden-runner) from 2.9.1 to 2.10.1. - [Release notes](https://github.com/step-security/harden-runner/releases) - [Commits](step-security/harden-runner@5c7944e...91182cc) --- updated-dependencies: - dependency-name: step-security/harden-runner dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#55220 Reviewed-By: Luigi Pinca <[email protected]>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 3.26.6 to 3.26.10. - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](github/codeql-action@4dd1613...e2b3eaf) --- updated-dependencies: - dependency-name: github/codeql-action dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#55221 Reviewed-By: Luigi Pinca <[email protected]>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 4.5.0 to 4.6.0. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@e28ff12...b9fd7d1) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> PR-URL: nodejs#55222 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55361 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#55273 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>
fix make errors that occur in coverage-clean case and coverage-test in Makefile PR-URL: nodejs#55287 Reviewed-By: LiviaMedeiros <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
update test_util.cc for code coverage src/util-inl.h:PopFront() PR-URL: nodejs#55291 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55312Fixes: nodejs#55311 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
PR-URL: nodejs#55116 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>
PR-URL: nodejs#55295 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55369 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Fixes: nodejs#23191 PR-URL: nodejs#55207 Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55375 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Claudio Wunder <[email protected]>
PR-URL: nodejs#55379 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
As the documentation states, the `context.importAssertion` should be still supported and emit a warning. This is true for the `load` hook, but not correct for context of the `resolve` hook. This commit fixes the inconsistency. PR-URL: nodejs#55365 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
This adds support for nodetimers.promises.scheduler.wait on Mocktimers Refs: nodejs#55244 PR-URL: nodejs#55244 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Chemi Atlow <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs#55187 Reviewed-By: Matteo Collina <[email protected]>
- The `Path` class does not support concatenation with the `+` operator, so use the `/` operator instead. - When concatenating paths, if the operand is an absolute path the previous path is ignored, so change `/include` to `include`. PR-URL: nodejs#55387 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fixes: nodejs#55391 PR-URL: nodejs#55392 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tim Perry <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Feng Yu <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
`test/parallel/test-dns-default-verbatim-false.js` is a duplicate of `test/parallel/test-dns-default-order-ipv4.js` and `test/parallel/test-dns-default-verbatim-true.js` is a duplicate of `test/parallel/test-dns-default-order-verbatim.js`. PR-URL: nodejs#55393 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Jake Yuesong Li <[email protected]>
The current colour seems something went wrong when in fact it's just someone asking for a review. PR-URL: nodejs#55423 Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Filip Skokan <[email protected]>
Refs: nodejs/Release#1042 PR-URL: nodejs#55399 Refs: nodejs/Release#1036 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Refs: nodejs/Release#1040 PR-URL: nodejs#55399 Refs: nodejs/Release#1036 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Refs: nodejs/Release#1041 PR-URL: nodejs#55399 Refs: nodejs/Release#1036 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Ulises Gascón <[email protected]> Reviewed-By: Ruy Adorno <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#55158 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]>
The two proposals reached stage 4 at the October 2024 meeting. PR-URL: nodejs#55333 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Backport-PR-URL: nodejs#55961
PR-URL: nodejs#55855 Refs: nodejs#55333 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Backport-PR-URL: nodejs#55961
PR-URL: nodejs#56706 Backport-PR-URL: nodejs#56721 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#56707 Backport-PR-URL: nodejs#56724 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
marco-ippolitoforce-pushed the v20.x-staging branch from 416c658 to cc7d1a7Compare
Original commit message: Reland "[cache] Don't compare host defined options if the script was deserialized" This is a reland of commit b9cfb8b7cfdbf195c3baf87735865948dfa5907e Original change's description: > [cache] Don't compare host defined options if the script was deserialized > > We don't serialize host defined options (see > CodeSerializer::SerializeObjectImpl()). > > Change-Id: Icab9fe910a5ec93b5eacc4869aba75034ad8b447 > Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4805329 > Reviewed-by: Camillo Bruni <[email protected]> > Reviewed-by: Toon Verwaest <[email protected]> > Commit-Queue: Tao Pan <[email protected]> > Cr-Commit-Position: refs/heads/main@{#90698} Change-Id: I7ea5e9355056104ebd25b385aba63c1233d42260 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4998581 Reviewed-by: Camillo Bruni <[email protected]> Reviewed-by: Toon Verwaest <[email protected]> Commit-Queue: Tao Pan <[email protected]> Cr-Commit-Position: refs/heads/main@{#90711} Refs: v8/v8@7b677a5
Original commit message: [compiler] support isolate compilation cache in CompileFunction() Previously there was no isolate compilation cache support for scripts compiled Script::CompileFunction() with wrapped arguments. This patch adds support for that. Refs: nodejs#35375 Change-Id: Id1849961ecd1282eb2dac95829157d167a3aa9a1 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4962094 Reviewed-by: Camillo Bruni <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#91681} Refs: v8/v8@f4b3f6e
Original commit message: [compiler] reset script details in functions deserialized from code cache During the serialization of the code cache, V8 would wipe out the host-defined options, so after a script id deserialized from the code cache, the host-defined options need to be reset on the script using what's provided by the embedder when doing the deserializing compilation, otherwise the HostImportModuleDynamically callbacks can't get the data it needs to implement dynamic import(). Change-Id: I33cc6a5e43b6469d3527242e083f7ae6d8ed0c6a Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5401780 Reviewed-by: Leszek Swirski <[email protected]> Commit-Queue: Joyee Cheung <[email protected]> Cr-Commit-Position: refs/heads/main@{#93323} Refs: v8/v8@cd10ad7 Co-authored-by: Joyee Cheung <[email protected]>
Original commit message: [snapshot] Check if a cached data has wrapped arguments Fixes that ScriptCompiler::CreateCodeCacheForFunction aborts on a deserialized shared function info from a cached data accepted with ScriptCompiler::CompileFunction. If the wrapped argument list does not match, the cached data should be rejected. Refs: nodejs#56366 Change-Id: I3f0376216791d866fac8eed1ce88dfa05e919b48 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6140933 Commit-Queue: Chengzhong Wu <[email protected]> Reviewed-by: Leszek Swirski <[email protected]> Cr-Commit-Position: refs/heads/main@{#97942} Refs: v8/v8@96ee9bb Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
Co-authored-by: Daniel Lemire <[email protected]> PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: nodejs#50322 Backport-PR-URL: nodejs#56590 Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
PR-URL: nodejs#52093 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Jacob Smith <[email protected]> Reviewed-By: Richard Lau <[email protected]>
Otherwise re-entering V8 doesn't work as expected after exceptions were thrown. Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5050065 Co-Authored-By: Toon Verwaest <[email protected]> Co-Authored-By: deepak1556 <[email protected]> PR-URL: nodejs#51362 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
This patch implements automatic on-disk code caching that can be enabled via an environment variable NODE_COMPILE_CACHE. When set, whenever Node.js compiles a CommonJS or a ECMAScript Module, it will use on-disk [V8 code cache][] persisted in the specified directory to speed up the compilation. This may slow down the first load of a module graph, but subsequent loads of the same module graph may get a significant speedup if the contents of the modules do not change. Locally, this speeds up loading of test/fixtures/snapshot/typescript.js from ~130ms to ~80ms. To clean up the generated code cache, simply remove the directory. It will be recreated the next time the same directory is used for `NODE_COMPILE_CACHE`. Compilation cache generated by one version of Node.js may not be used by a different version of Node.js. Cache generated by different versions of Node.js will be stored separately if the same directory is used to persist the cache, so they can co-exist. Caveat: currently when using this with V8 JavaScript code coverage, the coverage being collected by V8 may be less precise in functions that are deserialized from the code cache. It's recommended to turn this off when running tests to generate precise coverage. Implementation details: There is one cache file per module on disk. The directory layout is: - Compile cache directory (from NODE_COMPILE_CACHE) - 8b23c8fe: CRC32 hash of CachedDataVersionTag + NODE_VERESION - 2ea3424d: - 10860e5a: CRC32 hash of filename + module type - 431e9adc: ... - ... Inside the cache file, there is a header followed by the actual cache content: ``` [uint32_t] code size [uint32_t] code hash [uint32_t] cache size [uint32_t] cache hash ... compile cache content ... ``` When reading the cache file, we'll also check if the code size and code hash match the code that the module loader is loading and whether the cache size and cache hash match the file content read. If they don't match, or if V8 rejects the cache passed, we'll ignore the mismatch cache, and regenerate the cache after compilation succeeds and rewrite it to disk. PR-URL: nodejs#52535 Refs: nodejs#47472 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Instead of using an async function wrapper, just try compiling code with unknown module format as SourceTextModule when it cannot be compiled as CJS and the error message indicates that it's worth a retry. If it can be parsed as SourceTextModule then it's considered ESM. Also, move shouldRetryAsESM() to C++ completely so that we can reuse it in the CJS module loader for require(esm). Drive-by: move methods that don't belong to ContextifyContext out as static methods and move GetHostDefinedOptions to ModuleWrap. PR-URL: nodejs#52413 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Jacob Smith <[email protected]>
It might be worth designing a policy for the compilation cache. For now, just skip the cache when policy is enabled. PR-URL: nodejs#52577 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
Avoid repetitively calling into JS callback from C++ in `ModuleWrap::Link`. This removes the convoluted callback style of the internal `ModuleWrap` link step. PR-URL: nodejs#52058 Reviewed-By: Joyee Cheung <[email protected]>
This patch: 1. Adds ESM syntax detection to compileFunctionForCJSLoader() for --experimental-detect-module and allow it to emit the warning for how to load ESM when it's used to parse ESM as CJS but detection is not enabled. 2. Moves the ESM detection of --experimental-detect-module for the entrypoint from executeUserEntryPoint() into Module.prototype._compile() and handle it directly in the CJS loader so that the errors thrown during compilation *and execution* during the loading of the entrypoint does not need to be bubbled all the way up. If the entrypoint doesn't parse as CJS, and detection is enabled, the CJS loader will re-load the entrypoint as ESM on the spot asynchronously using runEntryPointWithESMLoader() and cascadedLoader.import(). This is fine for the entrypoint because unlike require(ESM) we don't the namespace of the entrypoint synchronously, and can just ignore the returned value. In this case process.mainModule is reset to undefined as they are not available for ESM entrypoints. 3. Supports --experimental-detect-module for require(esm). PR-URL: nodejs#52047 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
So that if there are circular dependencies in the synchronous module graph, they could be resolved using the cached jobs. In case linking fails and the error gets caught, reset the cache right after linking. If it succeeds, the caller will cache it again. Otherwise the error bubbles up to users, and since we unset the cache for the unlinkable module the next attempt would still fail. PR-URL: nodejs#52868Fixes: nodejs#52864 Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>
PR-URL: nodejs#53050 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: James M Snell <[email protected]>
Unifies the CJS and ESM source map cache map with SourceMapCacheMap and allows the CJS cache entries to be queried more efficiently with a source url without iteration on an IterableWeakMap. Add a test to verify that the CJS source map cache entry can be reclaimed. PR-URL: nodejs#51711 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs#52213 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
PR-URL: nodejs#52658 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Lemire <[email protected]>
joyeecheungforce-pushed the backport-require-esm-20 branch from a51b482 to 5d03fceComparemarco-ippolitoforce-pushed the v20.x-staging branch from cc7d1a7 to f78508cCompare
MemberAuthor
From my investigation in #56590 (comment) I feel like this strategy is probably riskier than backporting as few commits as I can, due to having commits like #50322 and #52135 in the way that caused a series of regressions and some of them aren't even fixed on main branch yet after months. Even though not backporting them could cause a lot of conflicts, at least I feel that I should know the commit essential to |
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is what I have so far by following the worklist in #52697 (comment) - not sure how we are going to manage the backport merges and whether this strategy is the best, but just opening a PR for now to leave some traces and get some discussion before I get too deep in the rabbit hole.
Notable things in this backport:
lib/internal/modules/,src/node_contextify.ccandsrc/module_wrap.cc, I am fairly certain all the relevant changes must have touched at least one of these), and picking all the commits that are affecting code paths ofrequire(esm), no matter they are directly done forrequire(esm)or not, to reduce conflicts and avoid introducing disconnected bits that might cause bugs. The commits that are not crossing paths withrequire(esm)under my assessment and the ones that obviously should not land (e.g. mass refactoring, semver-major) are omitted.module.dependencySpecifiers()(which is semver-major) and added a internal polyfill forFromV8Arrayin util.h since the new V8 API isn't on v20.x.internalRequire()which is still a thing on v20.x since policy is still there.require(esm)without also unflagging detect-module can be risky because the code are very intertwined, not just source-level but also logic-level.