fix(pnp): always initialize the ESM loader when enabled (#3667)

* fix: handle extensionless commonjs entrypoint * fix: set the main module when the entrypoint is commonjs * test: use fixture * test: fix check for unknown extensions * test: reuse fixture * test: remove old tests These tests checked that the code generation was correct but the code generation was replaced with a patch to the `fs` bindings * chore: make the type stricter * chore: versions * chore: changelog
yarnpkg · Nov 10, 2021 · 83311e1 · 83311e1
1 parent 5f08c44
commit 83311e1
Show file tree

Hide file tree

Showing 12 changed files with 212 additions and 306 deletions.
diff --git a/.pnp.cjs b/.pnp.cjs
diff --git a/.yarn/versions/4da93d59.yml b/.yarn/versions/4da93d59.yml
@@ -0,0 +1,28 @@
+releases:
+  "@yarnpkg/cli": patch
+  "@yarnpkg/plugin-pnp": patch
+  "@yarnpkg/pnp": patch
+
+declined:
+  - "@yarnpkg/esbuild-plugin-pnp"
+  - "@yarnpkg/plugin-compat"
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-nm"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnpm"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/builder"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"
+  - "@yarnpkg/nm"
+  - "@yarnpkg/pnpify"
+  - "@yarnpkg/sdks"
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -19,6 +19,7 @@ Yarn now accepts sponsorships! Please give a look at our [OpenCollective](https:
 ### Bugfixes
 
 - `@yarnpkg/pnpify` now escapes paths correctly
+- The ESM loader is now enabled regardless of the entrypoint module type, this fixes support for dynamic imports in commonjs modules when the entrypoint is also commonjs
 
 ### Bugfixes
 

diff --git a/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-bins-esm-1.0.0/index.mjs b/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-bins-esm-1.0.0/index.mjs
@@ -1 +1,2 @@
+import 'fs';
 console.log(42);
diff --git a/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-exports-1.0.0/index.js b/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-exports-1.0.0/index.js
diff --git a/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-exports-1.0.0/package.json b/packages/acceptance-tests/pkg-tests-fixtures/packages/no-deps-exports-1.0.0/package.json
diff --git a/packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts b/packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts
@@ -1,4 +1,4 @@
-import {Filename, PortablePath, ppath, xfs} from '@yarnpkg/fslib';
+import {Filename, ppath, xfs} from '@yarnpkg/fslib';
 
 describe(`Plug'n'Play - ESM`, () => {
   test(
@@ -141,57 +141,6 @@ describe(`Plug'n'Play - ESM`, () => {
     ),
   );
 
-  test(
-    `it should support named exports in commonjs files`,
-    makeTemporaryEnv(
-      {
-        dependencies: {
-          'no-deps-exports': `1.0.0`,
-        },
-        type: `module`,
-      },
-      async ({path, run, source}) => {
-        await expect(run(`install`)).resolves.toMatchObject({code: 0});
-
-        await xfs.writeFilePromise(
-          ppath.join(path, `index.js` as Filename),
-          `import {foo} from 'no-deps-exports';\nconsole.log(foo)`,
-        );
-
-        await expect(run(`node`, `./index.js`)).resolves.toMatchObject({
-          code: 0,
-          stdout: `42\n`,
-        });
-      },
-    ),
-  );
-
-  test(
-    `it should always set default as module.exports when importing a commonjs file`,
-    makeTemporaryEnv(
-      {
-        type: `module`,
-      },
-      async ({path, run, source}) => {
-        await expect(run(`install`)).resolves.toMatchObject({code: 0});
-
-        await xfs.writeFilePromise(
-          ppath.join(path, `index.js` as Filename),
-          `import foo from './foo.cjs';\nconsole.log(foo)`,
-        );
-        await xfs.writeFilePromise(
-          ppath.join(path, `foo.cjs` as Filename),
-          `module.exports.default = 42`,
-        );
-
-        await expect(run(`node`, `./index.js`)).resolves.toMatchObject({
-          code: 0,
-          stdout: `{ default: 42 }\n`,
-        });
-      },
-    ),
-  );
-
   test(
     `it should respect exports`,
     makeTemporaryEnv(
@@ -228,21 +177,17 @@ describe(`Plug'n'Play - ESM`, () => {
       {
         type: `module`,
         dependencies: {
-          foo: `portal:./pkg`,
+          'no-deps': `1.0.0`,
         },
       },
       async ({path, run, source}) => {
-        await xfs.mkdirPromise(ppath.join(path, `pkg` as Filename));
-        await xfs.writeJsonPromise(ppath.join(path, `pkg/package.json` as Filename), {});
-        await xfs.writeFilePromise(ppath.join(path, `pkg/index.js` as Filename), `module.exports = 42`);
-
-        await xfs.writeFilePromise(ppath.join(path, `index.js` as Filename), `import foo from 'foo';\nconsole.log(foo)`);
+        await xfs.writeFilePromise(ppath.join(path, `index.js` as Filename), `import pkg from 'no-deps';\nconsole.log(pkg)`);
 
         await expect(run(`install`)).resolves.toMatchObject({code: 0});
 
         await expect(run(`node`, `./index.js`)).resolves.toMatchObject({
           code: 0,
-          stdout: `42\n`,
+          stdout: `{ name: 'no-deps', version: '1.0.0' }\n`,
         });
       },
     ),
@@ -255,21 +200,21 @@ describe(`Plug'n'Play - ESM`, () => {
         type: `module`,
       },
       async ({path, run, source}) => {
-        await xfs.writeFilePromise(ppath.join(path, `index` as Filename), `console.log('foo')`);
+        await xfs.writeFilePromise(ppath.join(path, `index.ts` as Filename), ``);
 
         await expect(run(`install`)).resolves.toMatchObject({code: 0});
 
-        await expect(run(`node`, `./index`)).rejects.toMatchObject({
+        await expect(run(`node`, `./index.ts`)).rejects.toMatchObject({
           code: 1,
-          stderr: expect.stringContaining(`Unknown file extension ""`),
+          stderr: expect.stringContaining(`Unknown file extension ".ts"`),
         });
       },
     ),
   );
 
-  // Tests the workaround for https://github.com/nodejs/node/issues/33226
+  // Tests https://github.com/nodejs/node/issues/33226
   test(
-    `it should not enter ESM mode just because the loader is present`,
+    `it should not load extensionless commonjs files as ESM`,
     makeTemporaryEnv(
       { },
       {
@@ -289,37 +234,19 @@ describe(`Plug'n'Play - ESM`, () => {
   );
 
   test(
-    `it should enter ESM mode when entrypoint is ESM`,
+    `it should support ESM binaries`,
     makeTemporaryEnv(
       {
-        workspaces: [`workspace`],
         dependencies: {
-          pkg: `workspace:*`,
+          'no-deps-bins-esm': `1.0.0`,
         },
       },
       async ({path, run, source}) => {
-        await xfs.mkdirPromise(ppath.join(path, `workspace` as PortablePath));
-        await xfs.writeJsonPromise(ppath.join(path, `workspace/package.json` as PortablePath), {
-          name: `pkg`,
-          type: `module`,
-          bin: `index.mjs`,
-          peerDependencies: {
-            'no-deps': `*`,
-          },
-        });
-        await xfs.writeFilePromise(ppath.join(path, `workspace/index.mjs` as Filename), `import 'fs'; console.log('foo')`);
-
         await expect(run(`install`)).resolves.toMatchObject({code: 0});
 
-        // Ensure path is virtual (ie node can't find it by default)
-        await expect(run(`bin`, `pkg`)).resolves.toMatchObject({
+        await expect(run(`no-deps-bins-esm`)).resolves.toMatchObject({
           code: 0,
-          stdout: expect.stringContaining(`__virtual__`),
-        });
-
-        await expect(run(`pkg`)).resolves.toMatchObject({
-          code: 0,
-          stdout: `foo\n`,
+          stdout: `42\n`,
         });
       },
     ),
@@ -340,7 +267,7 @@ describe(`Plug'n'Play - ESM`, () => {
   );
 
   test(
-    `it should work with dynamic imports in esm mode`,
+    `it should support dynamic imports in ESM mode`,
     makeTemporaryEnv(
       {
         type: `module`,
@@ -360,21 +287,20 @@ describe(`Plug'n'Play - ESM`, () => {
     ),
   );
 
-  // Requires the ESM loader to be loaded but currently that enters ESM
-  // mode and would test the incorrect code path
-  test.skip(
-    `it should work with dynamic imports in commonjs mode`,
+  test(
+    `it should support dynamic imports in commonjs mode`,
     makeTemporaryEnv(
       {
         dependencies: {
           "no-deps": `1.0.0`,
+          "is-number": `1.0.0`,
         },
       },
       {
         pnpEnableEsmLoader: true,
       },
       async ({path, run, source}) => {
-        await xfs.writeFilePromise(ppath.join(path, `index.js` as Filename), `import('no-deps').then(() => console.log(42))`);
+        await xfs.writeFilePromise(ppath.join(path, `index.js` as Filename), `require('no-deps');\nimport('is-number').then(() => console.log(42))`);
 
         await expect(run(`install`)).resolves.toMatchObject({code: 0});
 
@@ -385,4 +311,33 @@ describe(`Plug'n'Play - ESM`, () => {
       },
     ),
   );
+
+  test(
+    `it should set the main module`,
+    makeTemporaryEnv(
+      {},
+      {
+        pnpEnableEsmLoader: true,
+      },
+      async ({path, run, source}) => {
+        await expect(run(`install`)).resolves.toMatchObject({code: 0});
+
+        await xfs.writeFilePromise(
+          ppath.join(path, `index.js` as Filename),
+          `
+            console.log({
+              require: typeof require,
+              main: require.main === module,
+              mainModule: process.mainModule === module,
+            });
+          `,
+        );
+
+        await expect(run(`node`, `index.js`)).resolves.toMatchObject({
+          code: 0,
+          stdout: `{ require: 'function', main: true, mainModule: true }\n`,
+        });
+      },
+    ),
+  );
 });
diff --git a/packages/yarnpkg-pnp/sources/esm-loader/built-loader.js b/packages/yarnpkg-pnp/sources/esm-loader/built-loader.js
diff --git a/packages/yarnpkg-pnp/sources/esm-loader/loaderUtils.ts b/packages/yarnpkg-pnp/sources/esm-loader/loaderUtils.ts
@@ -48,6 +48,9 @@ export function getFileFormat(filepath: string): string | null {
         `Unknown file extension ".json" for ${filepath}`,
       );
     }
+    // Matching files without extensions deviates from Node's default
+    // behaviour but is a fix for https://github.com/nodejs/node/issues/33226
+    case ``:
     case `.js`: {
       const pkg = nodeUtils.readPackageScope(filepath);
       if (pkg) {

diff --git a/packages/yarnpkg-pnp/sources/hook.js b/packages/yarnpkg-pnp/sources/hook.js
diff --git a/packages/yarnpkg-pnp/sources/loader/applyPatch.ts b/packages/yarnpkg-pnp/sources/loader/applyPatch.ts
@@ -126,10 +126,10 @@ export function applyPatch(pnpapi: PnpApi, opts: ApplyPatchOptions) {
 
     // Check if the module has already been created for the given file
 
-    const cacheEntry = entry.cache[modulePath] as PatchedModule;
+    const cacheEntry = entry.cache[modulePath] as PatchedModule | undefined;
     if (cacheEntry) {
-      // When a dynamic import is used in CJS files Node adds the module
-      // to the cache but doesn't load it so we do it here.
+      // When the Node ESM loader encounters CJS modules it adds them
+      // to the cache but doesn't load them so we do that here.
       //
       // Keep track of and check if the module is already loading to
       // handle circular requires.
@@ -139,6 +139,13 @@ export function applyPatch(pnpapi: PnpApi, opts: ApplyPatchOptions) {
       if (cacheEntry.loaded === false && cacheEntry.isLoading !== true) {
         try {
           cacheEntry.isLoading = true;
+
+          // The main module is exposed as a global variable
+          if (isMain) {
+            process.mainModule = cacheEntry;
+            cacheEntry.id = `.`;
+          }
+
           cacheEntry.load(modulePath);
         } finally {
           cacheEntry.isLoading = false;
@@ -155,8 +162,7 @@ export function applyPatch(pnpapi: PnpApi, opts: ApplyPatchOptions) {
 
     entry.cache[modulePath] = module;
 
-    // The main module is exposed as global variable
-
+    // The main module is exposed as a global variable
     if (isMain) {
       process.mainModule = module;
       module.id = `.`;
@@ -390,21 +396,5 @@ export function applyPatch(pnpapi: PnpApi, opts: ApplyPatchOptions) {
     return false;
   };
 
-  // Specifying the `--experimental-loader` flag makes Node enter ESM mode so we change it to not do that
-  // https://github.com/nodejs/node/blob/e817ba70f56c4bfd5d4a68dce8b165142312e7b6/lib/internal/modules/run_main.js#L72-L81
-  // Tested by https://github.com/yarnpkg/berry/blob/d80ee2dc5298d31eb864288d77671a2264713371/packages/acceptance-tests/pkg-tests-specs/sources/pnp-esm.test.ts#L226-L244
-  // Upstream issue https://github.com/nodejs/node/issues/33226
-  const originalRunMain = moduleExports.runMain;
-  moduleExports.runMain = function (main = process.argv[1]) {
-    const resolvedMain = nodeUtils.resolveMainPath(main);
-
-    const useESMLoader = resolvedMain ? nodeUtils.shouldUseESMLoader(resolvedMain) : false;
-    if (useESMLoader) {
-      originalRunMain(main);
-    } else {
-      Module._load(main, null, true);
-    }
-  };
-
   patchFs(fs, new PosixFS(opts.fakeFs));
 }
diff --git a/packages/yarnpkg-pnp/sources/loader/nodeUtils.ts b/packages/yarnpkg-pnp/sources/loader/nodeUtils.ts
@@ -7,37 +7,6 @@ const builtinModules = new Set(Module.builtinModules || Object.keys(process.bind
 
 export const isBuiltinModule = (request: string) => request.startsWith(`node:`) || builtinModules.has(request);
 
-// https://github.com/nodejs/node/blob/e817ba70f56c4bfd5d4a68dce8b165142312e7b6/lib/internal/modules/run_main.js#L11-L24
-export function resolveMainPath(main: NativePath) {
-  let mainPath = Module._findPath(npath.resolve(main), null, true);
-  if (!mainPath)
-    return false;
-
-  // const preserveSymlinksMain = getOptionValue(`--preserve-symlinks-main`);
-  // if (!preserveSymlinksMain)
-  mainPath = fs.realpathSync(mainPath);
-
-  return mainPath;
-}
-
-// https://github.com/nodejs/node/blob/e817ba70f56c4bfd5d4a68dce8b165142312e7b6/lib/internal/modules/run_main.js#L26-L41
-export function shouldUseESMLoader(mainPath: NativePath) {
-  // const userLoader = getOptionValue(`--experimental-loader`);
-  // if (userLoader)
-  //   return true;
-  // const esModuleSpecifierResolution =
-  //   getOptionValue(`--experimental-specifier-resolution`);
-  // if (esModuleSpecifierResolution === `node`)
-  //   return true;
-  // Determine the module format of the main
-  if (mainPath && mainPath.endsWith(`.mjs`))
-    return true;
-  if (!mainPath || mainPath.endsWith(`.cjs`))
-    return false;
-  const pkg = readPackageScope(mainPath);
-  return pkg && pkg.data.type === `module`;
-}
-
 // https://github.com/nodejs/node/blob/e817ba70f56c4bfd5d4a68dce8b165142312e7b6/lib/internal/modules/cjs/loader.js#L315-L330
 export function readPackageScope(checkPath: NativePath) {
   const rootSeparatorIndex = checkPath.indexOf(npath.sep);