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

Improve plugin resolution (fixes pnpm compatibility) #11248

Closed
wants to merge 0 commits into from
Closed

Improve plugin resolution (fixes pnpm compatibility) #11248

wants to merge 0 commits into from

Conversation

adamhl8
Copy link
Contributor

@adamhl8 adamhl8 commented Jul 23, 2021

Description

Fixes #8056

== See this reply for an explanation of the updated fix: #11248 (comment) ==

Previously, plugins were loaded from the node_modules directory that was parent to the current running instance of Prettier. Loading relative to the cwd ensures that the right node_modules is used (for example, when using Prettier with pnpm).

I added pkg-dir to handle finding the project directory. I initially wrote my own way to find it, but I figured using this package is probably better and safer than anything I could come up with. This package simply starts from the cwd and traverses up until it finds a path where package.json exists. Hopefully this wouldn't break plugin loading for anyone. I can't think of any project setups where this would.

I've tested this with my own projects that use pnpm and Prettier loads all my plugins without issue.

Checklist

  • I’ve added tests to confirm my change works.

I could be completely wrong, but I don't think additional tests need to be written for this change. All current tests pass. cwd will always be a path in the actual project that is being formatted, so the right node_modules will be found.

  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory).

I can update the docs if needed, but it seems like the way plugins.md is currently worded still makes sense.

Providing at least one path to `--plugin-search-dir`/`pluginSearchDirs`
turns off plugin autoloading in the default directory (i.e. `node_modules` above `prettier` binary).

The default directory is still the node_modules above the prettier binary, we're just getting there a different way.

  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@adamhl8 adamhl8 mentioned this pull request Jul 23, 2021
4 tasks
@fisker
Copy link
Member

fisker commented Jul 24, 2021

Thanks for working on this.

  1. We search project root differently in other place, see this
  2. If we decide to change search logic, what do you think just search from process.cwd()?
  3. Big problem, we still support global installation, this solution will break that, I'm not sure what's right to do.

@fisker
Copy link
Member

fisker commented Jul 24, 2021

This package simply starts from the cwd and traverses up until it finds a path where package.json exists. Hopefully this wouldn't break plugin loading for anyone. I can't think of any project setups where this would.

This will break monorepo.

@adamhl8
Copy link
Contributor Author

adamhl8 commented Jul 25, 2021

I've come up with a solution that fixes compatibility with pnpm and shouldn't (hopefully) break any project setups (monorepo, global install, etc). It also doesn't care about what the virtual store is called or where it is.

I added tests and updated the plugins documentation page as well. I'm not very familiar with jest so I'd appreciate if someone could check those over.

There are comments in the code that hopefully make it clear, but I'll somewhat briefly explain here:

Essentially, instead of trying to find pnpm's node_modules directory (which contains the symlinks), we just use the virtual store directly since this is where Prettier is being run from. (The virtual store contains hardlinks to the actual packages but that's irrelevant in this case.)

All packages in the virtual store have a directory structure that looks like this:

.pnpm/ (virtual store)
├── <package>@<version>/ (e.g. prettier@2.3.2)
│   └── node_modules/ (contains package + its dependencies)
│       ├── prettier/ (dir where prettier is run from)
│       │   ├── index.js
│       │   └── ...
│       ├── dependency1/
│       └── ...
├── prettier-plugin-foo@1.2.3
│   └── node_modules
│       └── prettier-plugin-foo
└── ...

All packages in the virtual store are appended with @<version>. We can check for that to determine if we're in the virtual store, move up into the parent directory (virtual store), and get the path for each plugin directory. Because each plugin directory will have its own node_modules, we can simply pass each path to the existing plugin resolution function.

Additionally, you can now pass --plugin-search-dir any arbitrary directory that contains Prettier plugins. This shouldn't affect anyone already loading plugins this way. i.e. you can pass node_modules directly or a directory that is the direct parent to plugins:

somePluginsDir/ (or node_modules)
├── prettier-plugin-foo/
└── prettier-plugin-bar/

@adamhl8 adamhl8 changed the title Load plugins relative to the cwd instead of relative to Prettier. Improve plugin resolution (fixes pnpm compatibility) Jul 25, 2021
@adamhl8

This comment was marked as outdated.

@malekjaroslav

This comment was marked as outdated.

@fisker

This comment was marked as outdated.

@adamhl8

This comment was marked as outdated.

docs/plugins.md Outdated Show resolved Hide resolved
docs/plugins.md Outdated Show resolved Hide resolved
src/common/load-plugins.js Outdated Show resolved Hide resolved
@adamhl8

This comment was marked as outdated.

@rburgst

This comment was marked as outdated.

@Shinigami92
Copy link
Member

any updates on this? I would love to be able to use pnpm

What's stopping you?
Just configure it like in this project: https://github.com/prettier/plugin-pug/blob/3c6dd7222b65fea5c2ba69ddbb00dbed56d70b86/.prettierrc.cjs#L8

@ZeroAurora
Copy link

any updates on this? I would love to be able to use pnpm

What's stopping you? Just configure it like in this project: https://github.com/prettier/plugin-pug/blob/3c6dd7222b65fea5c2ba69ddbb00dbed56d70b86/.prettierrc.cjs#L8

But it's something that should work out of box instead of manual configuring. Not to mention those who prefer to use JSON as the configuration format (like me).

@TicTak21

This comment was marked as outdated.

@adamhl8

This comment was marked as outdated.

@fisker

This comment was marked as outdated.

@adamhl8

This comment was marked as outdated.

@adamhl8

This comment was marked as outdated.

@adamhl8 adamhl8 mentioned this pull request Feb 7, 2022
src/common/load-plugins.js Outdated Show resolved Hide resolved
src/common/load-plugins.js Outdated Show resolved Hide resolved
@adamhl8
Copy link
Contributor Author

adamhl8 commented Aug 5, 2022

@fisker Was hoping you could take a look at this again. I fixed the merge conflicts and all tests pass.

@adamhl8
Copy link
Contributor Author

adamhl8 commented Oct 3, 2022

Added proper tests to make codecov happy. Are there any issues that are preventing this from being merged? @fisker

@fisker
Copy link
Member

fisker commented Oct 4, 2022

Sorry for the delay, I haven't take a deep look. But the plugin search related part already changed in next branch. Would like to change target base to next?

@adamhl8 adamhl8 closed this Oct 5, 2022
@adamhl8
Copy link
Contributor Author

adamhl8 commented Oct 5, 2022

@fisker I closed this by mistake. Are you able to reopen it and change the base to next?

I updated my changes/tests to work with next.

@fisker
Copy link
Member

fisker commented Oct 6, 2022

I can't, the button is disabled.

@adamhl8
Copy link
Contributor Author

adamhl8 commented Oct 6, 2022

Ah weird. New PR is here: #13583

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug?] Symlink'd plugins in node_modules are not auto detected.
7 participants