Skip to content

Commit 6e920f9

Browse files
authored
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.
1 parent ffc9f8a commit 6e920f9

File tree

9 files changed

+238
-87
lines changed

9 files changed

+238
-87
lines changed

.pnp.cjs

Lines changed: 25 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.pnp.loader.mjs

Lines changed: 63 additions & 26 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.yarn/versions/3fde6039.yml

Lines changed: 27 additions & 0 deletions
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

Lines changed: 32 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
1-
import {nodeUtils} from '@yarnpkg/core';
2-
import {Filename, npath, ppath, xfs} from '@yarnpkg/fslib';
3-
import {pathToFileURL} from 'url';
1+
import {nodeUtils} from '@yarnpkg/core';
2+
import {Filename, npath, ppath, xfs} from '@yarnpkg/fslib';
3+
import {HAS_LOADERS_AFFECTING_LOADERS} from '@yarnpkg/pnp/sources/esm-loader/loaderFlags';
4+
import {pathToFileURL} from 'url';
45

56
const ifAtLeastNode21It = nodeUtils.major >= 21 ? it : it.skip;
67
const ifAtMostNode20It = nodeUtils.major <= 20 ? it : it.skip;
@@ -777,6 +778,34 @@ describe(`Plug'n'Play - ESM`, () => {
777778
),
778779
);
779780

781+
// Tests /packages/yarnpkg-pnp/sources/esm-loader/fspatch.ts
782+
(HAS_LOADERS_AFFECTING_LOADERS ? it : it.skip)(
783+
`should support loaders importing named exports from commonjs files`,
784+
makeTemporaryEnv(
785+
{
786+
dependencies: {
787+
'no-deps-exports': `1.0.0`,
788+
},
789+
type: `module`,
790+
},
791+
async ({path, run, source}) => {
792+
await xfs.writeFilePromise(ppath.join(path, `loader.mjs`), `
793+
import {foo} from 'no-deps-exports';
794+
console.log(foo);
795+
`);
796+
await xfs.writeFilePromise(ppath.join(path, `index.js`), ``);
797+
798+
await expect(run(`install`)).resolves.toMatchObject({code: 0});
799+
800+
await expect(run(`node`, `--loader`, `./loader.mjs`, `./index.js`)).resolves.toMatchObject({
801+
code: 0,
802+
stdout: `42\n`,
803+
stderr: ``,
804+
});
805+
},
806+
),
807+
);
808+
780809
describe(`private import mappings`, () => {
781810
test(
782811
`it should support private import mappings`,

packages/yarnpkg-core/sources/worker-zip/index.js

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 1 addition & 1 deletion
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

Lines changed: 81 additions & 50 deletions
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

0 commit comments

Comments
 (0)