Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task
dbaeumer opened this issue Jun 12, 2024 · 14 comments
Open
1 task
Labels
bug Something isn't working

Comments

@dbaeumer
Copy link

dbaeumer commented Jun 12, 2024

Self-service

  • I'd be willing to implement a fix

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: microsoft/vscode-eslint#1856 (comment) 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
Copy link

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
Copy link
Member

arcanis commented Jun 13, 2024

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
Copy link

It works with node 18.18.2 (microsoft/vscode-eslint#1856 (comment))
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 (microsoft/vscode-eslint#1856 (comment))

@dbaeumer
Copy link
Author

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
Copy link
Author

Hangs for me under Windows as well.

@dbaeumer
Copy link
Author

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

@arcanis
Copy link
Member

arcanis commented Jun 13, 2024

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
Copy link
Member

arcanis commented Jun 14, 2024

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
Copy link

shermify commented Jun 14, 2024

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
Copy link
Member

arcanis commented Jun 17, 2024

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 nodejs/node#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 (un-ts/synckit#174) to suggest a workaround.

@kherock
Copy link
Contributor

kherock commented Jun 26, 2024

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

stylelint/vscode-stylelint#464 (comment)

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
Copy link

Hi @arcanis, Any updates on this? It seems the un-ts/synckit#174 issue has been fixed

@fmal
Copy link

fmal commented Jul 17, 2024

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants