Uses NODE_OPTIONS with PnP instead of CLI args #6629

arcanis · 2018-11-03T20:55:09Z

Summary

A common problem people currently have is with programs that call Node through the child_process interface but forget to forward the execArgv parameters. This morning alone there was three of them: @babel/node, node-sass, and node-gyp.

In order to alleviate this issue, this diff makes Yarn switch from passing the --require option explicitly through the command line to passing it via the NODE_OPTIONS environment variable. This should increase the compatibility at a low cost.

An alternative would be to monkey-patch the child_process module, but I'm strongly opposed to it - patching builtin modules might have serious consequences, and shouldn't be done only for the sake of convenience.

Since relying on the environment is sometimes tricky, I'd appreciate having a few set of eyes take a look at this diff before I merge it - just to be sure I don't miss something (cc @yarnpkg/core).

Also cc @jdalton (since that might maybe affect esm?)

Note: This PR has one problem, though: it might cause issues for people having spaces into their project path. This is caused by a bug/oversight in the way Node parses the NODE_OPTIONS value. I've work on a PR to fix it on their side (nodejs/node#24065), but that might cause some pain in the meantime. I could potentially add a warning when it happens, but it will still be displayed once the problem is solved - I'm not sure whether it's a good UX or not.

Test plan

Added tests, the existing one should pass as before

BYK

Looks good. I'd love to see two follow ups:

The additional tests I mentioned
Some fallback logic to the CLI args when we detect spaces in path. We can then limit this behavior to Node versions lower than the one with the fix (which is all of them at the moment). This should reduce the possibility of failures since I assume people having spaces in their paths on Windows (and potentially on macOS).

BYK · 2018-11-05T13:15:19Z

src/cli/commands/node.js

  if (await fs.exists(pnpPath)) {
-    args = ['-r', pnpPath, ...args];
+    nodeOptions += ` --require ${pnpPath}`;


I'm guessing the leading space wouldn't be an issue when NODE_OPTIONS is empty?

Yep, will work 👍

👆 It looks like

nodeOptions += ` --require ${pnpPath}`

should be

nodeOptions = `--require ${pnpPath} ${nodeOptions}`

so that pnp is the first preloaded module.

Update:

Issue filed #6941; PR #6942.

BYK · 2018-11-05T13:18:41Z

src/util/execute-lifecycle-script.js

@@ -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


Referring to your PR on Node would be nice. Also, would be great if we can centralize some of this PnP path logic.

I think I'll keep it duplicated for now - since it's very few lines, I'm concerned about over-abstracting it (since it's only used in two places and there are no reasons to think it will change)

BYK · 2018-11-05T13:22:14Z

src/util/execute-lifecycle-script.js

@@ -45,7 +37,6 @@ export async function getWrappersFolder(config: Config): Promise<string> {

  await makePortableProxyScript(process.execPath, wrappersFolder, {


Shall we unify this logic in the future and use node at all times?

What do you mean? At most we should replace makePortableScript with cmdShim (which already covers the behavior we're using here, I think)

BYK · 2018-11-05T13:27:12Z

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

+          });
+        },
+      ),
+    );


I think we need two more tests:

For cases where we spawn another script and purposefully omit node args which simulates the scenario you are solving for (is this the second test, I can't really tell?)

Have an already set NODE_OPTIONS env variable and make sure it is preserved.

The first test tests yarn node my-script.js', and the second tests yarn run my-script. I think that should cover the first case? Will add a test for empty NODE_OPTIONS 👍

buildsize · 2018-11-05T16:43:02Z

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	1.09 MB	1.09 MB	-390 bytes (0%)
yarn-[version].js	4.27 MB	4.27 MB	-1.06 KB (0%)
yarn-legacy-[version].js	4.45 MB	4.44 MB	-1.15 KB (0%)
yarn-v[version].tar.gz	1.1 MB	1.1 MB	-1.1 KB (0%)
yarn_[version]all.deb	804.98 KB	805.12 KB	140 bytes (0%)

Changelog tracks back up to 1.12.0 only. ## 1.12.3 **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. - Fixes an issue with `yarn audit` when using workspaces [6625](yarnpkg/yarn#6639) - [**Jeff Valore**](https://twitter.com/codingwithspike) - 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/node24065](nodejs/node#24065)) [6479](yarnpkg/yarn#6629) - [**Maël Nison**](https://twitter.com/arcanis) - Fixes Gulp when used with Plug'n'Play [6623](yarnpkg/yarn#6623) - [**Maël Nison**](https://twitter.com/arcanis) - Fixes an issue with `yarn audit` when the root package was missing a name [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder) - Fixes an issue with `yarn audit` when a package was depending on an empty range [6611](yarnpkg/yarn#6611) - [**Jack Zhao**](https://github.com/bugzpodder) - Fixes an issue with how symlinks are setup into the cache on Windows [6621](yarnpkg/yarn#6621) - [**Yoad Snapir**](https://github.com/yoadsn) - Upgrades `inquirer`, fixing `upgrade-interactive` for users using both Node 10 and Windows [6635](yarnpkg/yarn#6635) - [**Philipp Feigl**](https://github.com/pfeigl) - Exposes the path to the PnP file using `require.resolve('pnpapi')` [6643](yarnpkg/yarn#6643) - [**Maël Nison**](https://twitter.com/arcanis) ## 1.12.2 This release doesn't actually exists and was caused by a quirk in our systems. ## 1.12.1 - Ensures the engine check is ran before showing the UI for `upgrade-interactive` [6536](yarnpkg/yarn#6536) - [**Orta Therox**](https://github.com/orta) - Restores Node v4 support by downgrading `cli-table3` [6535](yarnpkg/yarn#6535) - [**Mark Stacey**](https://github.com/Gudahtt) - Prevents infinite loop when parsing corrupted lockfiles with unterminated strings [4965](yarnpkg/yarn#4965) - [**Ryan Hendrickson**](https://github.com/rhendric) - Environment variables now have to **start** with `YARN_` (instead of just contain it) to be considered [6518](yarnpkg/yarn#6518) - [**Michael Gmelin**](https://blog.grem.de) - Fixes the `extensions` option when used by `resolveRequest` [6479](yarnpkg/yarn#6479) - [**Maël Nison**](https://twitter.com/arcanis) - Fixes handling of empty string entries for `bin` in package.json [6515](yarnpkg/yarn#6515) - [**Ryan Burrows**](https://github.com/rhburrows) - Adds support for basic auth for registries with paths, such as artifactory [5322](yarnpkg/yarn#5322) - [**Karolis Narkevicius**](https://twitter.com/KidkArolis) - Adds 2FA (Two Factor Authentication) support to publish & alike [6555](yarnpkg/yarn#6555) - [**Krzysztof Zbudniewek**](https://github.com/neonowy) - Fixes how the `files` property is interpreted to bring it in line with npm [6562](yarnpkg/yarn#6562) - [**Bertrand Marron**](https://github.com/tusbar) - Fixes Yarn invocations on Darwin when the `yarn` binary was symlinked [6568](yarnpkg/yarn#6568) - [**Hidde Boomsma**](https://github.com/hboomsma) - Fixes `require.resolve` when used together with the `paths` option [6565](yarnpkg/yarn#6565) - [**Maël Nison**](https://twitter.com/arcanis) ## 1.12.0 - Adds initial support for PnP on Windows [6447](yarnpkg/yarn#6447) - [**John-David Dalton**](https://twitter.com/jdalton) - Adds `yarn audit` (and the `--audit` flag for all installs) [6409](yarnpkg/yarn#6409) - [**Jeff Valore**](https://github.com/rally25rs) - Adds a special logic to PnP for ESLint compatibility (temporary, until [eslint/eslint10125](eslint/eslint#10125) is fixed) [6449](yarnpkg/yarn#6449) - [**Maël Nison**](https://twitter.com/arcanis) - Makes the PnP hook inject a `process.versions.pnp` variable when setup (equals to `VERSIONS.std`) [6464](yarnpkg/yarn#6464) - [**Maël Nison**](https://twitter.com/arcanis) - Disables by default (configurable) the automatic migration of the `integrity` field. **It will be re-enabled in 2.0.** [6465](yarnpkg/yarn#6465) - [**Maël Nison**](https://twitter.com/arcanis) - Fixes the display name of the faulty package when the NPM registry returns corrupted data [6455](yarnpkg/yarn#6455) - [**Grey Baker**](https://github.com/greysteil) - Prevents crashes when running `yarn outdated` and the NPM registry forgets to return the `latest` tag [6454](yarnpkg/yarn#6454) - [**mad-mike**](https://github.com/mad-mike) - Fixes `yarn run` when used together with workspaces and PnP [6444](yarnpkg/yarn#6444) - [**Maël Nison**](https://twitter.com/arcanis) - Fixes an edge case when peer dependencies were resolved multiple levels deep (`webpack-dev-server`) [6443](yarnpkg/yarn#6443) - [**Maël Nison**](https://twitter.com/arcanis)

Uses NODE_OPTIONS when spawning a process

fcb9cfc

arcanis force-pushed the node-options branch from f8aed89 to fcb9cfc Compare November 3, 2018 20:57

arcanis mentioned this pull request Nov 5, 2018

yarn --pnp fails to install node-sass because of missing mkdirp #6613

Closed

BYK approved these changes Nov 5, 2018

View reviewed changes

Maël Nison and others added 3 commits November 5, 2018 15:15

Adds an additional test

8ab4cea

Update CHANGELOG.md

21c5092

Merge branch 'master' into node-options

ced838b

arcanis merged commit 7977c42 into yarnpkg:master Nov 5, 2018

edmorley mentioned this pull request Nov 15, 2018

"'node' is not recognized as an internal or external command" when using Yarn PnP on Windows #6683

Closed

arcanis mentioned this pull request Nov 27, 2018

Plug'n'Play: Error: Cannot find module #6626

Closed

arcanis mentioned this pull request Dec 29, 2018

Document Yarn Plug'n'Play yarnpkg/website#869

Closed

10 tasks

rwjblue mentioned this pull request Jan 18, 2019

Add support for Yarn PnP josdejong/workerpool#49

Closed

jdalton mentioned this pull request Jan 20, 2019

yarn pnp should be the first preloaded module in NODE_OPTIONS #6941

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uses NODE_OPTIONS with PnP instead of CLI args #6629

Uses NODE_OPTIONS with PnP instead of CLI args #6629

arcanis commented Nov 3, 2018

BYK left a comment

BYK Nov 5, 2018

arcanis Nov 5, 2018

jdalton Jan 20, 2019 •

edited

Loading

BYK Nov 5, 2018

arcanis Nov 5, 2018

BYK Nov 5, 2018

arcanis Nov 5, 2018

BYK Nov 5, 2018

arcanis Nov 5, 2018

buildsize bot commented Nov 5, 2018

		@@ -45,7 +37,6 @@ export async function getWrappersFolder(config: Config): Promise<string> {

		await makePortableProxyScript(process.execPath, wrappersFolder, {

Uses NODE_OPTIONS with PnP instead of CLI args #6629

Uses NODE_OPTIONS with PnP instead of CLI args #6629

Conversation

arcanis commented Nov 3, 2018

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buildsize bot commented Nov 5, 2018

jdalton Jan 20, 2019 •

edited

Loading