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

Use prettier from current workspace #29

Closed
vasa-chi opened this issue Jan 24, 2017 · 10 comments · Fixed by #39
Closed

Use prettier from current workspace #29

vasa-chi opened this issue Jan 24, 2017 · 10 comments · Fixed by #39
Labels
locked Please open a new issue and fill out the template instead of commenting.

Comments

@vasa-chi
Copy link
Contributor

Currently plugin uses internal dependency on prettier. I think it would be better to use prettier from current projects node_modules, if it exists.

@CiGit
Copy link
Member

CiGit commented Jan 29, 2017

ref #33

@esbenp
Copy link
Contributor

esbenp commented Jan 31, 2017

Makes sense I guess. As @CiGit laid out in #33 there could be a prioritization order a long the lines of.

  1. Use local version
  2. Use global version
  3. Use package version

As far as I can tell this is what is going on here https://github.com/Microsoft/vscode-eslint/blob/master/eslint-server/src/server.ts#L257

@dburles
Copy link

dburles commented Feb 17, 2017

Hey any update on this one @esbenp? I was just bitten by this after updating to 0.18.0.

@esbenp
Copy link
Contributor

esbenp commented Feb 17, 2017

Honestly, February is pretty booked with work, so wont have time until at the end. Any PRs are welcome though..

@CiGit
Copy link
Member

CiGit commented Feb 17, 2017

I'll certainly try to work on this in these upcoming days.
@esbenp Is it okay for you if there is a new configuration option to define the global installation folder(/usr/lib/node_modules/prettier for example) or would you prefer relying on PATH to find the global executable cli and then mess around to find the lib.
I see the later more difficult to implement correctly. It depends on OS/package manager (npm, yarn, ...) combination

@esbenp
Copy link
Contributor

esbenp commented Feb 21, 2017

Looking at https://github.com/Microsoft/vscode-eslint/blob/master/eslint-server/src/server.ts#L349 it seems they way they do it is to use Files of vscode-languageserver-node. You can see the method here https://github.com/Microsoft/vscode-languageserver-node/blob/8291f55041ea023c4acefa73d8f25f5384aa6426/server/src/files.ts#L178
Maybe just copy-paste resolveGlobalNodePath into our repo and use it to resolve the global node path in case no local is found? What do you think?

@CiGit
Copy link
Member

CiGit commented Feb 21, 2017

resolveGlobalNodePath, like requireg, are based on npm config get prefix which is only relevant to npm.
In prettier's readme, installation is first shown with yarn global add prettier.
But I get it, you prefer no additional config 😉 and I agree with that. I will just need to investigate some more.

@esbenp
Copy link
Contributor

esbenp commented Feb 21, 2017

Ah yeah I get what you mean now. Well, maybe there is nothing wrong with adding a path config and set it to null as default. If null, run the 1. local -> 2. npm global -> plugin. If not null, use that if executable is found or fall back to previous priority

@CiGit
Copy link
Member

CiGit commented Feb 21, 2017

Sorry if I was unclear.
If I get your idea correctly:

  1. try local
  2. try if(path) path resolve
  3. try npm global prefix
  4. bundled

Seems easy to implement but I find this weird as a user.

I've been thinking to alternatives:

  • Don't try to resolve any global install (there is currently no issue about a global resolution) and update this extension quickly/often enough. Updating it is required anyway when there is an API/config change.
  • Provide a command to update the bundled prettier npm i prettier@latest then reload vscode.
  • Spawn a prettier cli (which is found on the PATH) as a child process. (client / server)

@github-actions
Copy link

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot added the locked Please open a new issue and fill out the template instead of commenting. label Apr 13, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants