Uses NODE_OPTIONS with PnP instead of CLI args (#6629)

* Uses NODE_OPTIONS when spawning a process

* Adds an additional test

* Update CHANGELOG.md

CHANGELOG.md

@@ -4,6 +4,14 @@ Please add one entry in this file for each change in Yarn's behavior. Use the sa
 
 ## Master
 
+**Important:** This release contains a cache bump. It will cause the very first install following the upgrade to take slightly more time, especially if you don't use the [Offline Mirror](https://yarnpkg.com/blog/2016/11/24/offline-mirror/) feature. After that everything will be back to normal.
+
+- Uses `NODE_OPTIONS` to instruct Node to load the PnP hook, instead of raw CLI arguments
+
+  **Caveat:** This change might cause issues for PnP users having a space inside their cwd (cf [nodejs/node#24065](https://github.com/nodejs/node/pull/24065))
+
+  [#6479](https://github.com/yarnpkg/yarn/pull/6629) - [**Maël Nison**](https://twitter.com/arcanis)
+
 - Fixes Gulp when used with Plug'n'Play
 
   [#6623](https://github.com/yarnpkg/yarn/pull/6623) - [**Maël Nison**](https://twitter.com/arcanis)

packages/pkg-tests/pkg-tests-specs/sources/pnp.js

@@ -1340,5 +1340,74 @@ module.exports = makeTemporaryEnv => {
         },
       ),
     );
+
+    test(
+      `it should not break spawning new Node processes ('node' command)`,
+      makeTemporaryEnv(
+        {
+          dependencies: {[`no-deps`]: `1.0.0`},
+        },
+        {plugNPlay: true},
+        async ({path, run, source}) => {
+          await run(`install`);
+
+          await writeFile(`${path}/script.js`, `console.log(JSON.stringify(require('no-deps')))`);
+
+          await expect(
+            source(
+              `JSON.parse(require('child_process').execFileSync(process.execPath, [${JSON.stringify(
+                `${path}/script.js`,
+              )}]).toString())`,
+            ),
+          ).resolves.toMatchObject({
+            name: `no-deps`,
+            version: `1.0.0`,
+          });
+        },
+      ),
+    );
+
+    test(
+      `it should not break spawning new Node processes ('run' command)`,
+      makeTemporaryEnv(
+        {
+          dependencies: {[`no-deps`]: `1.0.0`},
+          scripts: {[`script`]: `node main.js`},
+        },
+        {plugNPlay: true},
+        async ({path, run, source}) => {
+          await run(`install`);
+
+          await writeFile(`${path}/sub.js`, `console.log(JSON.stringify(require('no-deps')))`);
+          await writeFile(
+            `${path}/main.js`,
+            `console.log(require('child_process').execFileSync(process.execPath, [${JSON.stringify(
+              `${path}/sub.js`,
+            )}]).toString())`,
+          );
+
+          expect(JSON.parse((await run(`run`, `script`)).stdout)).toMatchObject({
+            name: `no-deps`,
+            version: `1.0.0`,
+          });
+        },
+      ),
+    );
+
+    test(
+      `it should properly forward the NODE_OPTIONS environment variable`,
+      makeTemporaryEnv({}, {plugNPlay: true}, async ({path, run, source}) => {
+        await run(`install`);
+
+        await writeFile(`${path}/foo.js`, `console.log(42);`);
+
+        await expect(
+          run(`node`, `-e`, `console.log(21);`, {env: {NODE_OPTIONS: `--require ${path}/foo`}}),
+        ).resolves.toMatchObject({
+          // Note that '42' is present twice: the first one because Node executes Yarn, and the second one because Yarn spawns Node
+          stdout: `42\n42\n21\n`,
+        });
+      }),
+    );
   });
 };

packages/pkg-tests/yarn.test.js

@@ -21,7 +21,7 @@ const pkgDriver = generatePkgDriver({
   async runDriver(
     path,
     [command, ...args],
-    {cwd, projectFolder, registryUrl, plugNPlay, plugnplayShebang, plugnplayBlacklist},
+    {cwd, projectFolder, registryUrl, plugNPlay, plugnplayShebang, plugnplayBlacklist, env},
   ) {
     let beforeArgs = [];
     let middleArgs = [];
@@ -49,6 +49,7 @@ const pkgDriver = generatePkgDriver({
             [`PATH`]: `${path}/bin${delimiter}${process.env.PATH}`,
           },
           plugNPlay ? {[`YARN_PLUGNPLAY_OVERRIDE`]: plugNPlay ? `1` : `0`} : {},
+          env,
         ),
         cwd: cwd || path,
       },

src/cli/commands/node.js

@@ -21,14 +21,16 @@ export function hasWrapper(commander: Object, args: Array<string>): boolean {
 export async function run(config: Config, reporter: Reporter, flags: Object, args: Array<string>): Promise<void> {
   const pnpPath = `${config.lockfileFolder}/${PNP_FILENAME}`;
 
+  let nodeOptions = process.env.NODE_OPTIONS || '';
   if (await fs.exists(pnpPath)) {
-    args = ['-r', pnpPath, ...args];
+    nodeOptions += ` --require ${pnpPath}`;
   }
 
   try {
     await child.spawn(NODE_BIN_PATH, args, {
       stdio: 'inherit',
       cwd: flags.into || config.cwd,
+      env: {...process.env, NODE_OPTIONS: nodeOptions},
     });
   } catch (err) {
     throw err;

src/util/execute-lifecycle-script.js

@@ -26,14 +26,6 @@ export const IGNORE_MANIFEST_KEYS: Set<string> = new Set(['readme', 'notice', 'l
 // See https://github.com/yarnpkg/yarn/issues/2286.
 const IGNORE_CONFIG_KEYS = ['lastUpdateCheck'];
 
-async function getPnpParameters(config: Config): Promise<Array<string>> {
-  if (await fs.exists(`${config.lockfileFolder}/${constants.PNP_FILENAME}`)) {
-    return ['-r', `${config.lockfileFolder}/${constants.PNP_FILENAME}`];
-  } else {
-    return [];
-  }
-}
-
 let wrappersFolder = null;
 
 export async function getWrappersFolder(config: Config): Promise<string> {
@@ -45,7 +37,6 @@ export async function getWrappersFolder(config: Config): Promise<string> {
 
   await makePortableProxyScript(process.execPath, wrappersFolder, {
     proxyBasename: 'node',
-    prependArguments: [...(await getPnpParameters(config))],
   });
 
   await makePortableProxyScript(process.execPath, wrappersFolder, {
@@ -212,8 +203,9 @@ export async function makeEnv(
     }
   }
 
-  if (await fs.exists(`${config.lockfileFolder}/${constants.PNP_FILENAME}`)) {
-    const pnpApi = dynamicRequire(`${config.lockfileFolder}/${constants.PNP_FILENAME}`);
+  const pnpFile = `${config.lockfileFolder}/${constants.PNP_FILENAME}`;
+  if (await fs.exists(pnpFile)) {
+    const pnpApi = dynamicRequire(pnpFile);
 
     const packageLocator = pnpApi.findPackageLocator(`${config.cwd}/`);
     const packageInformation = pnpApi.getPackageInformation(packageLocator);
@@ -227,6 +219,11 @@ export async function makeEnv(
 
       pathParts.unshift(`${dependencyInformation.packageLocation}/.bin`);
     }
+
+    // Note that NODE_OPTIONS doesn't support any style of quoting its arguments at the moment
+    // For this reason, it won't work if the user has a space inside its $PATH
+    env.NODE_OPTIONS = env.NODE_OPTIONS || '';
+    env.NODE_OPTIONS += ` --require ${pnpFile}`;
   }
 
   pathParts.unshift(await getWrappersFolder(config));