Skip to content

Commit f8ceed4

Browse files
committed
fix(pnp) esm - support loaders importing named exports from commonjs (#5961)
**What's the problem this PR addresses?** nodejs/node#48842 changed it so that our fs patch is loaded after the internal translators so ESM importing named CJS exports in loaders doesn't work. Fixes #5951 **How did you fix it?** Updated our ESM loader patching to target the `fs` bindings used internally. **Checklist** - [x] I have read the [Contributing Guide](https://yarnpkg.com/advanced/contributing). - [x] I have set the packages that need to be released for my changes to be effective. - [x] I will check that all automated PR checks pass before the PR gets reviewed. (cherry picked from commit 6e920f9)
1 parent fd8cee6 commit f8ceed4

File tree

5 files changed

+144
-53
lines changed

5 files changed

+144
-53
lines changed

.yarn/versions/3fde6039.yml

+27
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
releases:
2+
"@yarnpkg/cli": patch
3+
"@yarnpkg/plugin-pnp": patch
4+
"@yarnpkg/pnp": patch
5+
6+
declined:
7+
- "@yarnpkg/plugin-compat"
8+
- "@yarnpkg/plugin-constraints"
9+
- "@yarnpkg/plugin-dlx"
10+
- "@yarnpkg/plugin-essentials"
11+
- "@yarnpkg/plugin-init"
12+
- "@yarnpkg/plugin-interactive-tools"
13+
- "@yarnpkg/plugin-nm"
14+
- "@yarnpkg/plugin-npm-cli"
15+
- "@yarnpkg/plugin-pack"
16+
- "@yarnpkg/plugin-patch"
17+
- "@yarnpkg/plugin-pnpm"
18+
- "@yarnpkg/plugin-stage"
19+
- "@yarnpkg/plugin-typescript"
20+
- "@yarnpkg/plugin-version"
21+
- "@yarnpkg/plugin-workspace-tools"
22+
- "@yarnpkg/builder"
23+
- "@yarnpkg/core"
24+
- "@yarnpkg/doctor"
25+
- "@yarnpkg/nm"
26+
- "@yarnpkg/pnpify"
27+
- "@yarnpkg/sdks"

packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts

+28
Original file line numberDiff line numberDiff line change
@@ -756,6 +756,34 @@ describe(`Plug'n'Play - ESM`, () => {
756756
),
757757
);
758758

759+
// Tests /packages/yarnpkg-pnp/sources/esm-loader/fspatch.ts
760+
(loaderFlags.HAS_LOADERS_AFFECTING_LOADERS ? it : it.skip)(
761+
`should support loaders importing named exports from commonjs files`,
762+
makeTemporaryEnv(
763+
{
764+
dependencies: {
765+
'no-deps-exports': `1.0.0`,
766+
},
767+
type: `module`,
768+
},
769+
async ({path, run, source}) => {
770+
await xfs.writeFilePromise(ppath.join(path, `loader.mjs` as Filename), `
771+
import {foo} from 'no-deps-exports';
772+
console.log(foo);
773+
`);
774+
await xfs.writeFilePromise(ppath.join(path, `index.js` as Filename), ``);
775+
776+
await expect(run(`install`)).resolves.toMatchObject({code: 0});
777+
778+
await expect(run(`node`, `--loader`, `./loader.mjs`, `./index.js`)).resolves.toMatchObject({
779+
code: 0,
780+
stdout: `42\n`,
781+
stderr: ``,
782+
});
783+
},
784+
),
785+
);
786+
759787
describe(`private import mappings`, () => {
760788
test(
761789
`it should support private import mappings`,

packages/yarnpkg-pnp/sources/esm-loader/built-loader.js

+1-1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/yarnpkg-pnp/sources/esm-loader/fspatch.ts

+81-50
Original file line numberDiff line numberDiff line change
@@ -4,61 +4,92 @@ import {HAS_LAZY_LOADED_TRANSLATORS} from './loaderFlags';
44

55
//#region ESM to CJS support
66
if (!HAS_LAZY_LOADED_TRANSLATORS) {
7-
/*
8-
In order to import CJS files from ESM Node does some translating
9-
internally[1]. This translator calls an unpatched `readFileSync`[2]
10-
which itself calls an internal `tryStatSync`[3] which calls
11-
`binding.fstat`[4]. A PR[5] has been made to use the monkey-patchable
12-
`fs.readFileSync` but assuming that wont be merged this region of code
13-
patches that final `binding.fstat` call.
14-
15-
1: https://github.com/nodejs/node/blob/d872aaf1cf20d5b6f56a699e2e3a64300e034269/lib/internal/modules/esm/translators.js#L177-L277
16-
2: https://github.com/nodejs/node/blob/d872aaf1cf20d5b6f56a699e2e3a64300e034269/lib/internal/modules/esm/translators.js#L240
17-
3: https://github.com/nodejs/node/blob/1317252dfe8824fd9cfee125d2aaa94004db2f3b/lib/fs.js#L452
18-
4: https://github.com/nodejs/node/blob/1317252dfe8824fd9cfee125d2aaa94004db2f3b/lib/fs.js#L403
19-
5: https://github.com/nodejs/node/pull/39513
20-
*/
21-
227
const binding = (process as any).binding(`fs`) as {
238
fstat: (fd: number, useBigint: false, req: any, ctx: object) => Float64Array;
9+
/**
10+
* Added in https://github.com/nodejs/node/pull/48658 / v20.5.0
11+
* Renamed in https://github.com/nodejs/node/pull/49593 / v20.8.0
12+
*/
13+
readFileSync?: (path: string, flag: number) => string;
14+
/**
15+
* Added in https://github.com/nodejs/node/pull/49593
16+
*/
17+
readFileUtf8?: (path: string, flag: number) => string;
2418
};
25-
const originalfstat = binding.fstat;
26-
27-
// Those values must be synced with packages/yarnpkg-fslib/sources/ZipOpenFS.ts
28-
const ZIP_MASK = 0xff000000;
29-
const ZIP_MAGIC = 0x2a000000;
3019

31-
binding.fstat = function(...args) {
32-
const [fd, useBigint, req] = args;
33-
if ((fd & ZIP_MASK) === ZIP_MAGIC && useBigint === false && req === undefined) {
20+
const originalReadFile = binding.readFileUtf8 || binding.readFileSync;
21+
if (originalReadFile) {
22+
// @ts-expect-error - No index signature
23+
binding[originalReadFile.name] = function (...args: Parameters<typeof originalReadFile>) {
3424
try {
35-
const stats = fs.fstatSync(fd);
36-
// The reverse of this internal util
37-
// https://github.com/nodejs/node/blob/8886b63cf66c29d453fdc1ece2e489dace97ae9d/lib/internal/fs/utils.js#L542-L551
38-
return new Float64Array([
39-
stats.dev,
40-
stats.mode,
41-
stats.nlink,
42-
stats.uid,
43-
stats.gid,
44-
stats.rdev,
45-
stats.blksize,
46-
stats.ino,
47-
stats.size,
48-
stats.blocks,
49-
// atime sec
50-
// atime ns
51-
// mtime sec
52-
// mtime ns
53-
// ctime sec
54-
// ctime ns
55-
// birthtime sec
56-
// birthtime ns
57-
]);
58-
} catch {}
59-
}
25+
return fs.readFileSync(args[0], {
26+
encoding: `utf8`,
27+
// @ts-expect-error - The docs says it needs to be a string but
28+
// links to https://nodejs.org/dist/latest-v20.x/docs/api/fs.html#file-system-flags
29+
// which says it can be a number which matches the implementation.
30+
flag: args[1],
31+
});
32+
} catch { }
6033

61-
return originalfstat.apply(this, args);
62-
};
34+
return originalReadFile.apply(this, args);
35+
};
36+
} else {
37+
/*
38+
In order to import CJS files from ESM Node does some translating
39+
internally[1]. This translator calls an unpatched `readFileSync`[2]
40+
which itself calls an internal `tryStatSync`[3] which calls
41+
`binding.fstat`[4]. A PR[5] has been made to use the monkey-patchable
42+
`fs.readFileSync` but assuming that wont be merged this region of code
43+
patches that final `binding.fstat` call.
44+
45+
1: https://github.com/nodejs/node/blob/d872aaf1cf20d5b6f56a699e2e3a64300e034269/lib/internal/modules/esm/translators.js#L177-L277
46+
2: https://github.com/nodejs/node/blob/d872aaf1cf20d5b6f56a699e2e3a64300e034269/lib/internal/modules/esm/translators.js#L240
47+
3: https://github.com/nodejs/node/blob/1317252dfe8824fd9cfee125d2aaa94004db2f3b/lib/fs.js#L452
48+
4: https://github.com/nodejs/node/blob/1317252dfe8824fd9cfee125d2aaa94004db2f3b/lib/fs.js#L403
49+
5: https://github.com/nodejs/node/pull/39513
50+
*/
51+
52+
const binding = (process as any).binding(`fs`) as {
53+
fstat: (fd: number, useBigint: false, req: any, ctx: object) => Float64Array;
54+
};
55+
const originalfstat = binding.fstat;
56+
57+
// Those values must be synced with packages/yarnpkg-fslib/sources/ZipOpenFS.ts
58+
const ZIP_MASK = 0xff000000;
59+
const ZIP_MAGIC = 0x2a000000;
60+
61+
binding.fstat = function (...args) {
62+
const [fd, useBigint, req] = args;
63+
if ((fd & ZIP_MASK) === ZIP_MAGIC && useBigint === false && req === undefined) {
64+
try {
65+
const stats = fs.fstatSync(fd);
66+
// The reverse of this internal util
67+
// https://github.com/nodejs/node/blob/8886b63cf66c29d453fdc1ece2e489dace97ae9d/lib/internal/fs/utils.js#L542-L551
68+
return new Float64Array([
69+
stats.dev,
70+
stats.mode,
71+
stats.nlink,
72+
stats.uid,
73+
stats.gid,
74+
stats.rdev,
75+
stats.blksize,
76+
stats.ino,
77+
stats.size,
78+
stats.blocks,
79+
// atime sec
80+
// atime ns
81+
// mtime sec
82+
// mtime ns
83+
// ctime sec
84+
// ctime ns
85+
// birthtime sec
86+
// birthtime ns
87+
]);
88+
} catch { }
89+
}
90+
91+
return originalfstat.apply(this, args);
92+
};
93+
}
6394
}
6495
//#endregion

packages/yarnpkg-pnp/sources/esm-loader/loaderFlags.ts

+7-2
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,14 @@ export const HAS_JSON_IMPORT_ASSERTION_REQUIREMENT = major > 17 || (major === 17
1212
// The message switched to using an array in https://github.com/nodejs/node/pull/45348
1313
export const WATCH_MODE_MESSAGE_USES_ARRAYS = major > 19 || (major === 19 && minor >= 2) || (major === 18 && minor >= 13);
1414

15-
// https://github.com/nodejs/node/pull/45659 changed the internal translators to be lazy loaded
15+
// https://github.com/nodejs/node/pull/45659 changed the internal translators to be lazy loaded so they use our patch.
16+
// https://github.com/nodejs/node/pull/48842 changed it so that our patch is loaded after the internal translators.
1617
// TODO: Update the version range if https://github.com/nodejs/node/pull/46425 lands.
17-
export const HAS_LAZY_LOADED_TRANSLATORS = major > 19 || (major === 19 && minor >= 3);
18+
export const HAS_LAZY_LOADED_TRANSLATORS = (major === 20 && minor < 6) || (major === 19 && minor >= 3);
19+
20+
// https://github.com/nodejs/node/pull/43772
21+
// TODO: Update the version range if it gets backported to v18.
22+
export const HAS_LOADERS_AFFECTING_LOADERS = major > 19 || (major === 19 && minor >= 6);
1823

1924
// https://github.com/nodejs/node/pull/42881
2025
export const ALLOWS_NON_FILE_PARENT = major > 18 || (major === 18 && minor >= 1) || (major === 16 && minor >= 17);

0 commit comments

Comments
 (0)