[Bug] `yarn add node` fails on PnP

JLHwung commented 4 years ago

[x] I'd be willing to implement a fix

Describe the bug

yarn add node@13.3.0 throws MODULE_NOT_FOUND on yarn v2

To Reproduce

await expect(packageJsonAndInstall({
  devDependencies: { node: `13.3.0`},
})).resolves.toBeTruthy();

Environment if relevant (please complete the following information): Sherlock

Yarn: v2

Additional context

The preinstall scripts of node@npm will invoke the binary node and generates /bin/node.js as its binary entry. It seems to me that Yarn incorrectly treat node in preinstall as a resolve request when this package provides node binary through the bin field.

It is now blocking babel's migration to yarn v2. I am willing to implement a fix if @arcanis can help me narrow down the issue.

nicolo-ribaudo commented 4 years ago

@JLHwung In case it can help you, this is a previous comment from @arcanis on this topic:

I found the problem with node ... since its package exposes a binary called node, Yarn is doing the nice thing and letting it use it inside its scripts ... so its preinstall script (node installArchSpecificPackage) calls the checked-in binary ... which doesn't exist yet, because it's installed at runtime

arcanis commented 4 years ago

Yep, it's a tricky one 🤔 It worked in npm / Yarn 1 because the shell only runs symlinks if the underlying file actually exists, whether in our case the bin initialisers are regular files.

It's not super pretty, but I think I'll special-case this one; it seems very unlikely it'll ever show up in another package.

yarnbot commented 4 years ago

This issue reproduces on master:

Error: expect(received).resolves.toBeTruthy()

Received promise rejected instead of resolved
Rejected to value: [Error: Command failed: /usr/bin/node /github/workspace/scripts/actions/../../packages/yarnpkg-cli/bundles/yarn.js install
]
    at expect (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-1.zip/node_modules/expect/build/index.js:138:15)
    at module.exports (evalmachine.<anonymous>:2:7)
    at executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:19)
    at executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:22)
    at Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:18)
    at ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:59)

yarnbot commented 4 years ago

This issue reproduces on master:

Error: expect(received).resolves.toBeTruthy()

Received promise rejected instead of resolved
Rejected to value: [Error: Command failed: /usr/bin/node /github/workspace/scripts/actions/../../packages/yarnpkg-cli/bundles/yarn.js install
]
    at expect (/github/workspace/.yarn/cache/expect-npm-24.8.0-8c7640c562-1.zip/node_modules/expect/build/index.js:138:15)
    at module.exports (evalmachine.<anonymous>:2:7)
    at executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:56:19)
    at executeInTempDirectory (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:17:22)
    at Object.executeRepro (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/executeRepro.js:24:18)
    at ExecCommand.execute (/github/workspace/.yarn/cache/@arcanis-sherlock-npm-1.0.20-06a8fabaa4-1.zip/node_modules/@arcanis/sherlock/lib/commands/exec.js:25:59)

arcanis commented 4 years ago

Looking at the node package in more detail, I'm not sure it'll be possible to make it compatible. The way it is written is very strange:

The postinstall calls node-bin-setup https://cdn.jsdelivr.net/npm/node@13.3.0/installArchSpecificPackage.js
Then node-bin-setup calls ... npm ... https://cdn.jsdelivr.net/npm/node-bin-setup@1.0.6/index.js

I guess the intent is to make the node package automatically install the architecture-specific package for node, but it's a really bad idea on two counts:

Since it doesn't try to be compatible with non-npm sources, it may corrupt their installs or prevent them to work altogether (like it does here - npm will have no idea how to install the architecture-specific package in a way compatible with PnP).
Even disregarding package manager compatibility, since it triggers an install while an install is already in progress (via the postinstall script), the package manager runs the risk of corrupting its own internal state. Modifying the node_modules layout while an install is ongoing is obviously a very unsupported thing¹ ...

Can I know what are exactly your requirements? The way Yarn is implemented, we already guarantee that the Node binary used by the scripts will be exactly the same one as the one used to run Yarn itself (it was already the case in the v1). Would using nvm or similar be ok for your use case?

¹ For example in Yarn 2 we write the build state into a file after all build scripts have executed. Due to how node-bin-setup works, it's quite likely we would end up running an infinite loop as the build state file wouldn't have been written yet when the package would get added, causing the nested install to re-trigger the postinstall for node, causing the package to be added again, causing an install to run again, etc.

JLHwung commented 4 years ago

Can I know what are exactly your requirements?

node@npm is required by compat-table, which is a devDepedency of preset-env. It is meant to build support data for compat-table.

AFAIK, the use case of node@npm in compat-table is to run JavaScript feature test on node as well as other engines. In this case the tested node need to be isolated with the test-runner node. nvm could solve this problem but node@npm looks more convenient.

Update: I have found a way around this issue: In https://github.com/babel/babel/pull/10873 I removed compat-table dependency and download the git repo to a subdirectory. So this issue is not blocking the migration in babel now, but it is still worthy to track here.

benjaminpreiss commented 1 year ago

Hey y'all

I am pretty clueless on how to resolve this currently - Can someone give me a hint?

Getting this error on our project:

# This file contains the result of Yarn building a package (node@npm:19.8.1)
# Script name: preinstall

node:internal/modules/cjs/loader:1093
  throw err;
  ^

Error: Cannot find module '/Users/***********/Documents/work/**********/node_modules/node/bin/node'
    at Module._resolveFilename (node:internal/modules/cjs/loader:1090:15)
    at Module._load (node:internal/modules/cjs/loader:934:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:83:12)
    at node:internal/main/run_main_module:23:47 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Node.js v19.8.1

Is there some kind of work around?

ziouf commented 3 months ago

Hello,

this is still an issue for me

# This file contains the result of Yarn building a package (node@npm:20.13.1)
# Script name: preinstall

node:internal/modules/cjs/loader:1148
  throw err;
  ^

Error: Cannot find module '/home/***/node_modules/node/bin/node'
    at Module._resolveFilename (node:internal/modules/cjs/loader:1145:15)
    at Module._load (node:internal/modules/cjs/loader:986:27)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49 {
  code: 'MODULE_NOT_FOUND',
  requireStack: []
}

Node.js v20.13.1

$ yarn --version
3.8.2

AdityaRanjanSingh commented 2 months ago

I was seeing the same error in my project. After some tries I found that there was a preinstall in package json which was failing for me it was husky was not able to find the .git directory. That should be the place to look for

yarnpkg / berry

[Bug] `yarn add node` fails on PnP #637