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

[edmorley]

Do you want to request a feature or report a bug?
Bug

What is the current behavior?
When using Plug and Play on Windows, the postinstall command of the package fetch-mock fails to run, when certain other packages are listed in dependencies - however runs fine when those other unrelated dependencies are removed (?!).

$ ./yarn-1.13.0-20181114.2216.js
yarn install v1.13.0-20181114.2216
warning package.json: No license field
info No lockfile found.
warning No license field
[1/5] Resolving packages...
[2/5] Fetching packages...
info fsevents@1.2.4: The platform "win32" is incompatible with this module.
info "fsevents@1.2.4" is an optional dependency and failed compatibility check. Excluding it from installation.
[3/5] Linking dependencies...
warning " > react2angular@4.0.4" has unmet peer dependency "@types/angular@>=1.5".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/prop-types@>=15".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/react@>=16".
warning " > react2angular@4.0.4" has unmet peer dependency "@types/react-dom@>=16".
[5/5] Building fresh packages...
error C:\Users\Ed\src\yarn-pnp-test\.pnp\unplugged\npm-fetch-mock-7.2.5-4682f51b9fa74d790e10a471066cb22f3ff84d48\node_modules\fetch-mock: Command failed.
Exit code: 1
Command: node scripts/support-fetch-mock.js
Arguments:
Directory: C:\Users\Ed\src\yarn-pnp-test\.pnp\unplugged\npm-fetch-mock-7.2.5-4682f51b9fa74d790e10a471066cb22f3ff84d48\node_modules\fetch-mock
Output:
'node' is not recognized as an internal or external command,
operable program or batch file.

If the current behavior is a bug, please provide the steps to reproduce.

1. mkdir test && cd test
2. curl https://nightly.yarnpkg.com/yarn-1.13.0-20181114.2216.js -O
3. Create a package.json with the following contents:

{
  "dependencies": {
    "@uirouter/angularjs": "0.4.3",
    "ajv": "6.5.5",
    "angular": "1.7.5",
    "angular-clipboard": "1.6.2",
    "angular-local-storage": "0.7.1",
    "angular1-ui-bootstrap4": "2.4.22",
    "auth0-js": "9.8.2",
    "bootstrap": "4.1.3",
    "d3": "5.7.0",
    "font-awesome": "4.7.0",
    "history": "4.7.2",
    "jquery": "3.3.1",
    "jquery.flot": "0.8.3",
    "jquery.scrollto": "2.1.2",
    "js-cookie": "2.2.0",
    "js-yaml": "3.12.0",
    "json-e": "2.7.1",
    "json-schema-defaults": "0.4.0",
    "lodash": "4.17.11",
    "metrics-graphics": "2.15.6",
    "moment": "2.22.2",
    "mousetrap": "1.6.2",
    "ng-text-truncate-2": "1.0.1",
    "numeral": "2.0.6",
    "popper.js": "1.14.5",
    "prop-types": "15.6.2",
    "query-string": "6.2.0",
    "react": "16.6.3",
    "react-day-picker": "7.2.4",
    "react-dom": "16.6.3",
    "react-fontawesome": "1.6.1",
    "react-highlight-words": "0.14.0",
    "react-hot-loader": "4.3.12",
    "react-hotkeys": "1.1.4",
    "react-lazylog": "3.1.4",
    "react-linkify": "0.2.2",
    "react-redux": "5.1.1",
    "react-router-dom": "4.3.1",
    "react-select": "2.1.1",
    "react-split-pane": "0.1.84",
    "react-table": "6.8.6",
    "react-tabs": "2.3.0",
    "react2angular": "4.0.4",
    "reactstrap": "6.5.0",
    "redux": "4.0.1",
    "redux-debounce": "1.0.1",
    "redux-thunk": "2.3.0",
    "taskcluster-client-web": "8.1.0",
    "taskcluster-lib-scopes": "10.0.1",
    "webpack": "4.25.1",
    "webpack-cli": "3.1.2"
  },
  "devDependencies": {
    "fetch-mock": "7.2.5"
  },
  "installConfig": {
    "pnp": true
  }
}

4. ./yarn-1.13.0-20181114.2216.js

What is the expected behavior?
That the yarn install succeed when using plug and play mode.

I've reduced the size of the testcase package.json a fair amount - however removing any more of the dependencies above resulted in the issue no longer occurring. I'm presuming there's a race condition or other oddity occurring?

Please mention your node.js, yarn and operating system version.

• Windows 10
• Node.js 11.0.0
• Yarn Nightly v1.13.0-20181114.2216

cc @arcanis

[edmorley]

The --verbose output gave the following stack:

'node' is not recognized as an internal or external command,
operable program or batch file.
    at ChildProcess.proc.on (C:\Users\Ed\src\yarn-1.13.0-20181114.2216.js:25288:15)
    at ChildProcess.emit (events.js:182:13)
    at maybeClose (internal/child_process.js:962:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:251:5)

Annotating Yarn's spawn wrapper showed that program, args and env were virtually identical between the working (ie some packages removed from dependencies) and not working cases, except in the broken case env.PATH contained additional entries.

The size of stringified env in a working case was 16530 characters, and 16689 in the broken case.
For env.PATH, the stringified lengths were 8740 and 8899 characters respectively.

According to this page, the maximum size of PATH on windows when used by the command prompt, is 8192 characters:
https://support.microsoft.com/en-gb/help/830473/command-prompt-cmd-exe-command-line-string-limitation

...which seems to line up with the findings above (once taking into account the additional characters added by JSON.stringify()).

There are also some other limits discussed here:
https://superuser.com/questions/1070272/why-does-windows-have-a-limit-on-environment-variables-at-all

ie: The additional directories on PATH push the original OS PATH entries past the point that gets truncated, such that node then cannot be found.

Do all of those package directories need to be on PATH? I see entries that don't have binaries, that perhaps shouldn't be there? eg:
C:\\Users\\Ed\\AppData\\Local\\Yarn\\Cache\\v4\\npm-react-16.6.3-25d77c91911d6bbdd23db41e70fb094cc1e0871c\\node_modules\\react/.bin

[edmorley]

Do all of those package directories need to be on PATH? I see entries that don't have binaries, that perhaps shouldn't be there?

These appear to come from here:

yarn/src/util/execute-lifecycle-script.js

Lines 213 to 221 in 7f41910

	for (const [name, reference] of packageInformation.packageDependencies.entries()) {
	const dependencyInformation = pnpApi.getPackageInformation({name, reference});
	if (!dependencyInformation || !dependencyInformation.packageLocation) {
	continue;
	}
	pathParts.unshift(`${dependencyInformation.packageLocation}/.bin`);
	}

One possible approach would be to check for the existence of a .bin directory within each package on the filesystem at lifecycle script runtime (to determine which should be added to PATH) - however that would presumably be slow. Perhaps the .pnp.js metadata could instead be updated to track which packages have declared binaries, that way allowing makeEnv() to filter packageInformationStores directly?

[arcanis]

Oh wow, thanks for the debug! Completely forgot about the PATH size limits 😞

I think on the short term I'll do the existsSync call, but long term I'm considering deprecating the "binaries-exposed-through-the-PATH behavior" in favor of a single run utility that would have pretty much the same behavior than yarn run or npm run, but in a much more portable fashion:

{
  "scripts": {
    "foo": "run babel --preset=env [...]"
  }
}

[hWorblehat]

Hi @arcanis,

Any movement on this? I'm hitting it. Would you be opposed to me submitting a PR for checking fot the existence of a .bin subdirectory, at least as a temporary fix?

I don't see an easy way around the resulting performnce hit (although it didn't seem that bad to me when I tested). Maybe we could only perform the checking if we're on Windows?

[hWorblehat]

Incidentally, I like your idea of having a run utility that would bootstrap Node binaries. I do forsee a problem though:

Like you said, we would have to deprecate the old behaviour, rather than remove it, to avoid breaking existing projects. That means this bug would still exist - adding the run command doesn't fix anything, as the PATH will still get corrupted on Windows for being too long.

Not sure what the alternative is...

[arcanis]

Any movement on this? I'm hitting it. Would you be opposed to me submitting a PR for checking fot the existence of a .bin subdirectory, at least as a temporary fix?

Absolutely not, please do! I think the perf impact should be negligible - this only affects scripts, which are supposed to be very few. It should be fine to do it on every platform for now.

Do you think you can open the PR today? Otherwise I can give it a look as well.

That means this bug would still exist - adding the run command doesn't fix anything, as the PATH will still get corrupted on Windows for being too long.

Yep, the run command would be a related improvement, not a fix in itself. The best fix right now is to check for the .bin existence, as you mentioned.

[hWorblehat]

Do you think you can open the PR today? Otherwise I can give it a look as well.

Yep - working on it now 😄 Just testing the changes...

[hWorblehat]

Yep, the run command would be a related improvement, not a fix in itself.

I must be missing something then - what benefit would the run command provide?

[arcanis]

It would normalize how scripts and binaries are called across package managers (less so for binaries because for now the PATH injection works fine, but it might cause issues in the future when we implement zip loading - we won't be able to put those zip paths into the PATH).

One particular example is this:

{
  "scripts": {
    "build": "npm run foo && npm run bar",
    "build:foo": "do-something foo",
    "build:bar": "do-something bar"
  }
}

In the example above, Yarn will not be used to run build:foo and build:bar even if you use yarn build (because the npm binary is hardcoded into the script). That can potentially cause problems since it's possible that npm won't have all the knowledge required to correctly execute those scripts.

Similarly, if you replace yarn by npm in those scripts, then yarn will always be used even if you use npm run build, which is obviously not what you intended to.

Having a run indirection allows all package managers to implement it however they want, as long as they agree on the general CLI interface. Scripts would then become package-manager-agnostic.

[hWorblehat]

Ah - I get it now. Makes sense.

[hWorblehat]

I've raised PR #6711 to 'fix' this. As mentioned in the PR, it really just makes this issue less likely to occur, rather than avoiding it entirely.