fix: pnpm is already installed in the current workspace and does not work. #1475

Dunqing · 2022-05-30T06:22:23Z

close: #1474

dbaeumer

Should we not first try locally and then globally?

Dunqing · 2022-05-30T14:12:27Z

Should we not first try locally and then globally?

No, pnpm root will return an path, whether it exists locally or not

itpropro · 2022-06-10T17:50:20Z

Any progress on this? I have the same error as mentioned in #1474 and this would fix it 😉

dbaeumer · 2022-06-13T07:05:52Z

Sorry, has to wait until next week. Busy with other things.

ivangabriele · 2022-06-18T10:35:35Z

server/src/eslint.ts

+					try {
+						pnpmPath = execSync('pnpm root -g').toString().trim();
+					} catch {
+						pnpmPath = execSync('pnpm root').toString().trim();


I guess you enclosed that as a catch to avoid any behavior breaking change. But unless we want to make that a customizable settings to force global pnpm use instead of local one, I don't think this is necessary, is it?

I would simply remove the -g flag from the original source.

see #1475 (comment)

That's exactly what I'm saying ;). Since pnpm root will always return the path, there is no need to keep the pnpm root -g line.

That's exactly what I'm saying ;). Since pnpm root will always return the path, there is no need to keep the pnpm root -g line.

When you don't have pnpm installed locally, executing pnpm root will return a non-existent path, which will cause a more serious error.

Unless there is a better way to fix this problem, we must use pnpm root -g as a preference.

@Dunqing What about checking if the local path exists and falling back to pnpm root -g? 🤔

@Dunqing What about checking if the local path exists and falling back to pnpm root -g? 🤔

Sounds good.

dbaeumer · 2022-08-04T13:37:52Z

Sorry for the long delay.

Disclaimer: I have little knowledge about pnpm.

When you are talking about having pnpm install locally then you mean in a node_modules folder. Right?

If this is the case then the proposed PR will not work since execSync will not look into the ./node_modules/.bin folder.

If this is incorrect what does it mean to have pnpm installed locally?

Dunqing · 2022-08-11T13:31:16Z

When you are talking about having pnpm install locally then you mean in a node_modules folder. Right?

Yes.

If this is the case then the proposed PR will not work since execSync will not look into the ./node_modules/.bin folder.

I don't think it's related to execSync .

dbaeumer · 2022-08-12T08:25:17Z

But how should this find pnpm then ?

pnpmPath = execSync('pnpm root').toString().trim()

Dunqing · 2022-08-13T06:12:03Z

pnpmPath = execSync('pnpm root').toString().trim()

This is correct, but we need to check if the path exists #1475 (comment).

If it doesn't exist, we run pnpm root -g again .

Or you can work with the current PR approach.

dbaeumer · 2022-08-15T09:38:09Z

May be I still misunderstand this. In the example you gave me pnpm seems to be installed both globally and locally. If I only install pnpm locally and run it from a terminal I get a Command 'pnpm' not found message.

So this will not work if pnpm isn't installed globally as well. Right? And could this lead to some version mismatch where the global pnpm version is different than the local one ?

Dunqing · 2022-08-15T13:32:33Z

May be I still misunderstand this. In the example you gave me pnpm seems to be installed both globally and locally.

I have only installed globally.

If I only install pnpm locally and run it from a terminal I get a Command 'pnpm' not found message.

Yes, that's right. Because this PR is just to solve the issue of #1474.

So this will not work if pnpm isn't installed globally as well. Right?

Yes, this is the expected behavior, or we can finally fallback to npx pnpm .

And could this lead to some version mismatch where the global pnpm version is different than the local one ?

Yes, but there shouldn't be any problems.

We should actually use the local pnpm first, but at the moment i don't know if we can tell if the local pnpm is available.

dbaeumer · 2022-08-16T08:38:38Z

There is actually a way to find this out. Have a look at:

vscode-eslint/server/src/eslint.ts

Line 833 in 8eeb994

    
           promise = Files.resolve('eslint', settings.resolvedGlobalPackageManagerPath, moduleResolveWorkingDirectory, trace);

This is how I find the eslint installation and File.resolve considers the node resolution rules.

tiagobento · 2022-09-01T18:08:17Z

Is it possible to add a configuration option for users to manually configure where the extension should look for the eslint installation? Something like "eslint.installationPath": "node_modules/.pnpm/eslint@7.26.0/node_modules/eslint"

dbaeumer · 2022-09-02T07:43:55Z

This more or less exist with the eslint.nodePath setting. When looking for eslint that path is considered as well.

bene-we · 2022-09-12T13:19:08Z

Hi all, I ran into the problem with pnpm. On startup, the extension would throw Error: Command failed: pnpm root -g, yet I made sure that all paths are setup correctly, and pnpm root and pnpm root -g were working correctly in all terminals (including the VS Code terminal).

After searching for a while and going through this discussion, I removed the -g from the built eslintServer.js file, the eslint extension is now able to find the local bin directory and is starting correctly. I have pnpm installed only globally (using volta)

@dbaeumer would it be worth to build and try the try ... catch from this PR and test if it works on my machine? Would that approval be sufficient to merge this?

Before:

[Info  - 3:15:16 PM] ESLint server is starting
[Info  - 3:15:16 PM] ESLint server running in node v16.14.2
[Info  - 3:15:16 PM] ESLint server is running.
Uncaught exception received.
Error: Command failed: pnpm root -g
    at checkExecSyncError (node:child_process:848:11)
    at execSync (node:child_process:922:15)
    at node:electron/js2c/asar_bundle:5:12704
    at Object.get (<path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:23876)
    at e.get (<path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:23985)
    at <path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:19323
Uncaught exception received.
Error: Command failed: pnpm root -g
    at checkExecSyncError (node:child_process:848:11)
    at execSync (node:child_process:922:15)
    at node:electron/js2c/asar_bundle:5:12704
    at Object.get (<path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:23876)
    at e.get (<path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:23985)
    at <path-to-user>/.vscode/extensions/dbaeumer.vscode-eslint-2.2.6/server/out/eslintServer.js:1:19323

After:

[Info  - 3:08:38 PM] ESLint server is starting
[Info  - 3:08:38 PM] ESLint server running in node v16.14.2
[Info  - 3:08:38 PM] ESLint server is running.
[Info  - 3:08:42 PM] ESLint library loaded from: <path-to-project>/react-frontend/node_modules/.pnpm/registry.npmjs.org+eslint@8.23.0/node_modules/eslint/lib/api.js

dbaeumer · 2022-09-14T10:39:47Z

@bene-we

I tried a clean pnpm install and running pnpm root -g give me this:

ERROR  Unable to find the global bin directory
Run "pnpm setup" to create it automatically, or set the global-bin-dir setting, or the PNPM_HOME env variable. The global bin directory should be in the PATH.

I think that is the reason why the call actually fails.

The reason why the eslint server wants the global installation directory is to be able to find eslint if it is not installed locally. So removing the -g is actually not what I want :-).

Can someone example to me why pnpm needs additional setup for a global module location?

dbaeumer · 2022-09-14T10:41:11Z

After running pnpm setup pnpm root -g works as expected.

bene-we · 2022-09-14T12:01:53Z

I tried a clean pnpm install and running pnpm root -g give me this:
ERROR  Unable to find the global bin directory
Run "pnpm setup" to create it automatically, or set the global-bin-dir setting, or the PNPM_HOME env variable. The global bin directory should be in the PATH.
I think that is the reason why the call actually fails.

@dbaeumer experienced this as well, I wrote my comment after fixing it using pnpm setup. Yet the problem still exists even if pnpm root -g works in all terminals but in eslint extension.

Is there anything that could help find the root cause for it? Some logging for instance?

dbaeumer · 2022-09-16T08:23:35Z

You could try "eslint.trace.server": "verbose"

If this doesn't help I can provide you with steps how to debug this if you are willing to do so.

bene-we · 2022-09-16T11:59:43Z

@dbaeumer just enabled the tracing, and voilà magically it also works with pnp root -g in eslintServer.js. I even uninstalled and reinstalled the VS Code extension in order to have the original build files. Seems like VS Code or the Eslint extension just needed a restart (which I didn't try after I encountered the issue).

For anyone else: Please try to get pnpm root -g to work in all terminals and do a fresh restart of your computer, seems like the issue is fixed with that. If not, feel free to comment in here so I could try to debug further.

latin-1 · 2022-11-28T13:04:43Z

I'm sorry, but are there any updates?

dbaeumer · 2022-12-05T19:44:53Z

I will close the PR for now. As it turned out pnpm needs an additional setup step to ensure pnpm root -g works without failure. But this is necessary for the extension to work correctly.

Please ping if someone has an idea to avoid the additional setup step.

fix: pnpm is already installed in the current workspace

74c7dae

Dunqing force-pushed the fix-workspace-pnpm branch from b9dc4fd to 74c7dae Compare May 30, 2022 06:23

dbaeumer reviewed May 30, 2022

View reviewed changes

Dunqing requested a review from dbaeumer June 11, 2022 01:48

ivangabriele reviewed Jun 18, 2022

View reviewed changes

snyk-bot mentioned this pull request Aug 21, 2022

[Snyk] Upgrade ts-loader from 6.2.1 to 9.3.1 meonBot/vscode-eslint#8

Open

dbaeumer closed this Dec 5, 2022

dbaeumer mentioned this pull request Dec 5, 2022

packageManager set to pnpm will not work #1474

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: pnpm is already installed in the current workspace and does not work. #1475

fix: pnpm is already installed in the current workspace and does not work. #1475

Dunqing commented May 30, 2022

dbaeumer left a comment

Dunqing commented May 30, 2022

itpropro commented Jun 10, 2022

dbaeumer commented Jun 13, 2022

ivangabriele Jun 18, 2022

Dunqing Jun 18, 2022

ivangabriele Jun 18, 2022

Dunqing Jun 18, 2022

JanJakes Aug 2, 2022

Dunqing Aug 11, 2022

dbaeumer commented Aug 4, 2022

Dunqing commented Aug 11, 2022

dbaeumer commented Aug 12, 2022

Dunqing commented Aug 13, 2022

dbaeumer commented Aug 15, 2022

Dunqing commented Aug 15, 2022

dbaeumer commented Aug 16, 2022

tiagobento commented Sep 1, 2022 •

edited

Loading

dbaeumer commented Sep 2, 2022

bene-we commented Sep 12, 2022

dbaeumer commented Sep 14, 2022

dbaeumer commented Sep 14, 2022

bene-we commented Sep 14, 2022

dbaeumer commented Sep 16, 2022

bene-we commented Sep 16, 2022

latin-1 commented Nov 28, 2022

dbaeumer commented Dec 5, 2022

fix: pnpm is already installed in the current workspace and does not work. #1475

fix: pnpm is already installed in the current workspace and does not work. #1475

Conversation

Dunqing commented May 30, 2022

dbaeumer left a comment

Choose a reason for hiding this comment

Dunqing commented May 30, 2022

itpropro commented Jun 10, 2022

dbaeumer commented Jun 13, 2022

ivangabriele Jun 18, 2022

Choose a reason for hiding this comment

Dunqing Jun 18, 2022

Choose a reason for hiding this comment

ivangabriele Jun 18, 2022

Choose a reason for hiding this comment

Dunqing Jun 18, 2022

Choose a reason for hiding this comment

JanJakes Aug 2, 2022

Choose a reason for hiding this comment

Dunqing Aug 11, 2022

Choose a reason for hiding this comment

dbaeumer commented Aug 4, 2022

Dunqing commented Aug 11, 2022

dbaeumer commented Aug 12, 2022

Dunqing commented Aug 13, 2022

dbaeumer commented Aug 15, 2022

Dunqing commented Aug 15, 2022

dbaeumer commented Aug 16, 2022

tiagobento commented Sep 1, 2022 • edited Loading

dbaeumer commented Sep 2, 2022

bene-we commented Sep 12, 2022

dbaeumer commented Sep 14, 2022

dbaeumer commented Sep 14, 2022

bene-we commented Sep 14, 2022

dbaeumer commented Sep 16, 2022

bene-we commented Sep 16, 2022

latin-1 commented Nov 28, 2022

dbaeumer commented Dec 5, 2022

tiagobento commented Sep 1, 2022 •

edited

Loading