[v22.x backport] fs: restore fs patchability in ESM loader - #64722
[v22.x backport] fs: restore fs patchability in ESM loader#64722ash2228 wants to merge 1 commit into
Conversation
|
Review requested:
|
avivkeller
left a comment
There was a problem hiding this comment.
While intentionally adding a thing we don't recommend is odd to me, LGTM to unbreak consumers
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
I have not managed to get a successful Jenkins run so far, however there have been general problems this week, so I would wait until the security release has gone out before coming back to this PR. |
|
Thanks for the update, I'll wait until the security release is out and then revisit this PR. Appreciate you taking a look. |
|
nodejs/citgm#1112 shows Yarn 4.18.0 failing in CITGM tests (without this PR). It's possible that there are multiple issues contributing to failure. |
a6b3a35 to
400e833
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Temporarily restore fs patchability in ESM loader as a workaround for helping downstream projects that depend on this undocumented hidden contract transition into using hook proper APIs. This patch intentionally avoids adding a test and instead adds warning comments to hopefully steer new code away from depending on it. PR-URL: nodejs#62835 Refs: nodejs#62012 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> (cherry picked from commit 9531947)
40e2961 to
7dd23a7
Compare
|
Thanks, I rebased the PR onto the latest v22.x-staging (currently based on 400e833). Let me know if there's anything else I should update. |
Backport of #62835 to the v22.x line.
#62835 landed on
mainin 9531947 and was backported to v26.x (v26.1.0)and v24.x (v24.16.0), but not to v22.x. It did not cherry-pick cleanly onto
v22 —
lib/internal/modules/esm/load.jsconflicted, which is likely why itwas missed.
This fixes #64709. Under Yarn PnP on v22.22.3 through v22.23.1, the ESM
loader captures
readFileSync/realpathSyncbefore PnP patchesfs, soNode cannot read sources from the zip cache itself. PnP supplies the source
through its load hook instead, which routes loading through the synthetic
require in
loadCJSModule— that one does not setcacheorextensions.Result:
rechoir/webpack-cliandimport-fresh/postcss-loaderbothbreak.
Conflict resolution: v22's
load.jsalso importsgetOptionValuefor--experimental-default-type, which does not exist on the newer lines. Thatimport is preserved; only the
fsimport and call sites changed. The restThe rest applied cleanly from 9531947.
The v22 build is still compiling locally, so I haven't yet run the
reproducer against a patched binary — I'll follow up with before/after
output. Opening now so it's visible in case someone wants to pick it up
sooner.