From fcb9cfc2744bc758a51b0246b9b6aa67d544a069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Sat, 3 Nov 2018 13:43:17 -0700 Subject: [PATCH 1/3] Uses NODE_OPTIONS when spawning a process --- .../pkg-tests/pkg-tests-specs/sources/pnp.js | 53 +++++++++++++++++++ src/cli/commands/node.js | 4 +- src/util/execute-lifecycle-script.js | 19 +++---- 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js index ec95577320..9b469ed334 100644 --- a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js +++ b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js @@ -1340,5 +1340,58 @@ 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`, + }); + }, + ), + ); }); }; diff --git a/src/cli/commands/node.js b/src/cli/commands/node.js index a8c752c1a1..1fb70fd1ee 100644 --- a/src/cli/commands/node.js +++ b/src/cli/commands/node.js @@ -21,14 +21,16 @@ export function hasWrapper(commander: Object, args: Array): boolean { export async function run(config: Config, reporter: Reporter, flags: Object, args: Array): Promise { 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; diff --git a/src/util/execute-lifecycle-script.js b/src/util/execute-lifecycle-script.js index 82c01acae3..7401cc10e8 100644 --- a/src/util/execute-lifecycle-script.js +++ b/src/util/execute-lifecycle-script.js @@ -26,14 +26,6 @@ export const IGNORE_MANIFEST_KEYS: Set = new Set(['readme', 'notice', 'l // See https://github.com/yarnpkg/yarn/issues/2286. const IGNORE_CONFIG_KEYS = ['lastUpdateCheck']; -async function getPnpParameters(config: Config): Promise> { - 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 { @@ -45,7 +37,6 @@ export async function getWrappersFolder(config: Config): Promise { 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)); From 8ab4ceabc4ad2bfdc9a03bd6efceadb1744af86a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 5 Nov 2018 15:15:17 +0000 Subject: [PATCH 2/3] Adds an additional test --- .../pkg-tests/pkg-tests-specs/sources/pnp.js | 16 ++++++++++++++++ packages/pkg-tests/yarn.test.js | 3 ++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js index 9b469ed334..487669f959 100644 --- a/packages/pkg-tests/pkg-tests-specs/sources/pnp.js +++ b/packages/pkg-tests/pkg-tests-specs/sources/pnp.js @@ -1393,5 +1393,21 @@ module.exports = makeTemporaryEnv => { }, ), ); + + 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`, + }); + }), + ); }); }; diff --git a/packages/pkg-tests/yarn.test.js b/packages/pkg-tests/yarn.test.js index 25b5d968f0..284b57ee61 100644 --- a/packages/pkg-tests/yarn.test.js +++ b/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, }, From 21c50929de054bf88d59cd970f1531deda0e2247 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Mon, 5 Nov 2018 16:40:04 +0000 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index edb1a86287..49c7baa984 100644 --- a/CHANGELOG.md +++ b/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) + ## 1.12.1 - Ensures the engine check is ran before showing the UI for `upgrade-interactive`