yarnpkg / berry

πŸ“¦πŸˆ Active development trunk for Yarn βš’
https://yarnpkg.com
BSD 2-Clause "Simplified" License
7.44k stars 1.11k forks source link

[Bug?]: Yarn pnp fails with synckit and prettier plugin in eslint context #6335

Open dbaeumer opened 5 months ago

dbaeumer commented 5 months ago

Self-service

Describe the bug

Yarn pnp can't load synckit worker hence prettier plugin fails to run.

Why do I file this against yarn? Everything works as expected when installing the dependencies with npm.

To reproduce

Use NodeJS 20.9.0

Clone https://github.com/shermify/eslint-plugin-repro

> yarn install
> export NODE_PATH=".yarn/sdks"

Setting the NODE_PATH is equivalent to setting the VS Code eslint.nodePath setting.

start node REPL and execute the following commands (to ensure to mimic the executing since a VS Code extension):

require.resolve('eslint');

We will load the eslint npm module from .yarn/sdks

const library = require('eslint');
async function main() { const eslint = new (await library.loadESLint())(); const report = await eslint.lintText(`var test = "hello";`); console.log(report); }
main().then(() => { console.log('Promise resolved'); } ).catch((error) => { console.error(error); process.exitCode = 1; });

Observe: the promise never resolves. You can't even terminate NodeJS using Ctrl+C. You have to kill it.

I debugged it in the context of the VS Code ESLint extension and the findings are here: https://github.com/microsoft/vscode-eslint/issues/1856#issuecomment-2162280404 and in the following comments.

Environment

System: OS: Linux 5.15 Debian GNU/Linux 11 (bullseye) 11 (bullseye) CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700K Binaries: Node: 20.9.0 - /tmp/xfs-426f4fe7/node Yarn: 4.3.0 - /tmp/xfs-426f4fe7/yarn npm: 10.1.0 - ~/.nvs/default/bin/npm pnpm: 9.1.1 - /usr/local/share/npm-global/bin/pnpm

Additional context

No response

shermify commented 5 months ago

The problem seems to be a dynamic import in worker.js in the package eslint-plugin-prettier. I tried a few fixes and I could only get it to work using require. Dynamic import seems to lock up the node process.

await import('prettier') /fail await import('prettier/index.cjs') /fail (await import('prettier')).default /fail const prettier = require('prettier') /works

arcanis commented 5 months ago

Observe: the promise never resolves. You can't even terminate NodeJS using Ctrl+C. You have to kill it.

I don't repro that; is something missing? πŸ€”

Screenshot 2024-06-13 at 10 47 14

Outside of that, did you try with a newer Node release? If the whole process ends up locked it could be a Node.js bug.

joonaathaan commented 5 months ago

It works with node 18.18.2 (https://github.com/microsoft/vscode-eslint/issues/1856#issuecomment-2162347554) I've tested with 18.20.0; 20.9.0; 20.14.0 (LTS) and i can still reproduce it. if i set nodeLinker: node-modules the promise gets resolved, doesn't matter which node version it is in this case. Doesn't that mean it's yarn pnp related?

I'm not sure if these debug logs are helpful but this is what i did as well (https://github.com/microsoft/vscode-eslint/issues/1856#issuecomment-2160957041)

dbaeumer commented 5 months ago

I tested this again and the steps provided makes it hang for me. I tested this under Ubuntu 22.04 in case this makes a difference.

dbaeumer commented 5 months ago

Hangs for me under Windows as well.

dbaeumer commented 5 months ago

@arcanis is there anything I can capture that might help you to understand what is going on?

arcanis commented 5 months ago

if i set nodeLinker: node-modules the promise gets resolved, doesn't matter which node version it is in this case. Doesn't that mean it's yarn pnp related?

Yes and no. The Node.js ESM loader implementation is quite fluctuating, so bugs sometimes appear and get fixed on later versions (for instance some 22.x versions introduce a deadlock with some other loaders). As a result problems are very rarely about PnP itself, but more the APIs we're given to integrate with ESM in Node.

is there anything I can capture that might help you to understand what is going on?

I managed to repro - I was running yarn node (which translates into node --require ./.pnp.cjs --loader ./.pnp.loader.mjs), but in your repro it's just a good old node. It shouldn't make a difference, but perhaps Node regressed and is somehow registering the loaders twice even when they have the same path? Investigating.

arcanis commented 5 months ago

The relevant error here is visible when running yarn unplug eslint-plugin-prettier inside the repository. After that, the REPL starts to properly reject the promise with an ERR_MODULE_NOT_FOUND not found.

Digging into the source code, I see that this error is thrown by a worker thread spawned just so eslint-plugin-prettier can synchronously and conditionally require prettier. The SDK doesn't currently inject the --require and --loader flag in a way that would work through nested workers, so that explains the issue. I'll look what we can do.

As for the deadlock, I have no idea - given that it works after running yarn unplug (ie after materializing the eslint-plugin-prettier files on disk, rather than by loading them from their zip archive) I suspect something in the Node.js error handling code is trying to access the files on disk and doesn't support memory modules, which seems a bug to me.

shermify commented 5 months ago

Do you know why, when it's unplugged, the dynamic import still doesn't seem to work but it does work when you change it to require? I would guess that it still needs a module loader in scope in both cases. Also, if you change it to require and eslint-plugin-prettier is still "plugged", you still get the deadlock. So something about unplugging it allows you to access the commonjs loader? Or possibly prettier (unplugged) can only dynamic import other unplugged modules? Apologies if I'm missing something obvious here, this is a bit outside my wheelhouse :)

arcanis commented 5 months ago

Do you know why, when it's unplugged, the dynamic import still doesn't seem to work but it does work when you change it to require?

Because the Node.js ESM loader pipeline is still broken 😞

Digging into the issue, it seems the --loader flag that SyncKit adds is ignored by the new Worker constructor, which seems to be the subject of https://github.com/nodejs/node/issues/47747. That's what causes the ERR_MODULE_NOT_FOUND error when dynamically importing prettier, which in turns causes a deadlock probably because of a faulty error handling code somewhere in the Node.js internals.

I opened an issue on SyncKit (https://github.com/un-ts/synckit/issues/174) to suggest a workaround.

kherock commented 4 months ago

sorry if this is just noise, but I believe I'm seeing a similar issue when comsiconfig attempts to dynamic import a stylelint plugin:

https://github.com/stylelint/vscode-stylelint/issues/464#issuecomment-2174603745

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/Users/herockk/Workspaces/example/.yarn/__virtual__/stylelint-prettier-virtual-4b73a1c895/3/.yarn/berry/cache/stylelint-prettier-npm-5.0.0-f95c073012-10c0.zip/node_modules/stylelint-prettier/recommended.js' imported from /Users/herockk/Workspaces/example/.yarn/__virtual__/cosmiconfig-virtual-a7403f58a3/3/.yarn/berry/cache/cosmiconfig-npm-9.0.0-47d78cf275-10c0.zip/node_modules/cosmiconfig/dist/loaders.js
    at new NodeError (node:internal/errors:406:5)
    at finalizeResolution (node:internal/modules/esm/resolve:233:11)
    at moduleResolve (node:internal/modules/esm/resolve:852:10)
    at defaultResolve (node:internal/modules/esm/resolve:1050:11)
    at nextResolve (node:internal/modules/esm/hooks:833:28)
    at resolve$1 (file:///Users/herockk/Workspaces/example/.pnp.loader.mjs:1976:12)
    at nextResolve (node:internal/modules/esm/hooks:833:28)
    at Hooks.resolve (node:internal/modules/esm/hooks:278:30)
    at MessagePort.handleMessage (node:internal/modules/esm/worker:168:24)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:807:20)

The module loads when stylelint-prettier is unplugged.

agarwalvaibhav0211 commented 4 months ago

Hi @arcanis, Any updates on this? It seems the https://github.com/un-ts/synckit/issues/174 issue has been fixed

fmal commented 4 months ago

@agarwalvaibhav0211 yes it has been fixed with latest release of synckit (https://github.com/un-ts/synckit/releases/tag/v0.9.1) which is a dependency of eslint-plugin-prettier. Just make sure you have synckit@0.9.1 in your packages. If reinstalling eslint-plugin-prettier package doesn't fetch latest synckit then you can enforce it through yarnresolutions like this:

"resolutions": {
  "synckit": "^0.9.1"
}
hanyufoodles commented 4 months ago

eslint-plugin-prettier@5.2.1 has bumped synckit to 0.9.1