Plugin autoload doesn't work for packages installed from GitHub and for Yarn PnP
fisker opened this issue · 28 comments
Environments:
- Prettier Version: 2.0.5
- Running Prettier via: CLI
- Runtime: Node.js v14
- Operating System: Windows
Steps to reproduce:
yarn add prettier/prettier @prettier/plugin-pugExpected behavior:
Can format pug file with npx prettier test.pug
Actual behavior:
The CLI can't load pug plugin automatically
Problem should be here, when install from github, ./node_modules/prettier maybe has own node_modules dir, so the autoLoadDir will be ./node_modules/prettier, not expected ./, so we are looking for ./node_modules/prettier/node_modules/@prettier/plugin-* not ./node_modules/@prettier/plugin-* (where the @prettier/plugin-pug is installed).
Relabeling with "area:api" as this isn't specific to the CLI.
How do we fix it?
I was thinking check the dirname is prettier, but seems not safe, the working dir could be prettier, installed package maybe not prettier (when use yarn add foo@prettier/prettier), mark something in the package.json?
Checking package.json to see what plugins are installed might work.
The old logic didn't require it to be listed in package.json, it could installed by prettier-plugins(this package contains all plugins he use), or prettier could be installed globally.
What do you think, use pkg-dir to get the prettier root, and find from it's parent, this comment also says logic is in this way.
The only thing will break is we can't test plugin by adding @prettier/plugin-pug in our codebase.
const packageRoot = packageDir(__dirname);
const autoLoadDir = thirdParty.findParentDir(
path.join(packageRoot, ".."),
"node_modules"
);The old logic didn't require it to be listed in package.json, it could installed by prettier-plugins(this package contains all plugins he use),
Such a package must follow the plugin naming scheme too.
or prettier could be installed globally.
Does the bug that we discuss here affect global installation?
Such a package must follow the plugin naming scheme too
I don't think so, if this package has lots plugins in dependencies, we will load.
Does the bug that we discuss here affect global installation?
Hmm, not sure.
I don't think so, if this package has lots plugins in dependencies, we will load.
I didn't understand what you mean then. I thought you were talking about something like "plugin packs" - packages that aren't plugins by themselves but install multiple other plugins. Apparently, you meant something else. Please explain.
BTW, did you get my email? Can you open DMs on Twitter?
I thought you were talking about something like "plugin packs" - packages that aren't plugins by themselves but install multiple other plugins.
That's what I mean, "plugin packs" don't need named prettier-plugin-* or @prettier/plugin-*. So, we can't
Checking package.json to see what plugins are installed
So far Prettier didn't promise to support such plugin packs. They might get installed in such a way that there will be an inner node_modules too. Prettier won't be able to find plugins installed to that inner node_modules. Only direct dependencies are guaranteed to end up in the top-level node_modules. That's why I think plugin packs should behave like plugins and follow the naming scheme for plugins. Also I still think Prettier, unless it's installed globally, should use package.json to discover installed plugins. Not instead of the current behavior but in addition to it.
So far Prettier didn't promise to support such plugin packs. They might get installed in such a way that there will be an inner node_modules too. Prettier won't be able to find plugins installed to that inner node_modules. Only direct dependencies are guaranteed to end up in the top-level node_modules. That's why I think plugin packs should behave like plugins and follow the naming scheme for plugins. Also I still think Prettier, unless it's installed globally, should use package.json to discover installed plugins. Not instead of the current behavior but in addition to it.
I think we have a wrong logic here right now https://github.com/prettier/prettier/blob/master/src/common/load-plugins.js.
Plugin(s) should be added to package.json. For loading plugins we should use require. Ideally we should not glob node_modules. Just note - Yarn PnP, doesn't have node_modules.
Eslint looking at a configuration to find out which plugins are installed. But we 0CJS, so some plugins can't be listed in the configuration. package.json only one place to get list of plugins. But we should not forget package.json also may not exist, for example - Deno. In this case I think we should implement --plugin flag for CLI (like do Eslint).
My vision:
- Load
package.jsonto get list of plugins (they should beprettier-plugin-*/@prettier/plugin-*). - Using
requireto load them. - Implement
--pluginoption for nonpackage.jsonenvironments and for projects which doesn't havepackage.json.
Pros:
- Simple logic.
- Does not overload filesystem for searching plugins on each start.
- Supports environments without
package.json. - Yarn PnP works fine.
Cons:
- We don't need
--plugin-search-dir, so we should remove it and it is breaking change. But we can implement it without breaking change. - Can be broken for some developers, because developer can have a package with
prettier-plugin-nameindependencies, now we load a plugin, after this we can't load. But we can keep globbing fromnode_modulesto avoid breaking change.
These are my thoughts on this issue, I would like to hear other opinions.
Maybe we should take a look at Eslint logic. It is pretty well designed.
In this case I think we should implement --plugin flag for CLI
We have it. https://prettier.io/docs/en/plugins.html#using-plugins
@thorn0 Strange, I see it for the first time 😄 But I think it should support --plugin=name, without full path, I am from mobile, so hard to look at source code.
@evilebottnawi It supports that. It accepts what require accepts.
I like the idea to check listed plugins in package.json, but I don't like only load these plugins.
If I want @prettier/plugin-pug, I prefer add it to my @fisker/prettier-config instead of adding to project package.json.
I prefer auto load
- plugins in
package.json - plugins in
.prettierrc(No support yet)
Side suggestion: adding prettierrc → plugins field: #7073 (comment)
Happy to create a new issue / PR if there are no major flaws in the concept.
I like plugins, I have one concern, what if it's a ES Module, we'll need use import() instead of require() to load it, but if only filepath in plugins, we can't know what is it, so do we try both require/import? Or any better idea?
We havn't support esm plugin, but we will face it.
ESLint folks are simplifying their config via eslint/eslint#13481 and they have some nice thoughts in https://github.com/eslint/rfcs/tree/master/designs/2019-config-simplification. Essentially, they no longer have paths in their config and the plugins field is now a lookup. A key is the name of the plugin and the value is an object/function that represents it. I.e. it is the actual plugin, not a path to it.
Wondering if we could do something similar in Prettier. Is it necessary that the resulting Prettier config is serializable? Even if not, a Yarn-2-compatible config would need to be in js to be able to call require() or require.resolve() (or even import()?).
Is it necessary that the resulting Prettier config is serializable?
Personally I use .js , but I feel there will be objections.
It’s already possible to declare config → plugins and that works with Yarn PnP 😳 🎉
It’s already possible to declare
config→pluginsand that works with Yarn PnP 😳 🎉 #7073 (comment)
Doing it this way, the glob pattern resolving stuff will not detect file extensions declared by the plugins. This is because the context object (which resolves the glob into filepaths) is loaded before the .prettierrc file (see #13276). As a result the plugins get loaded due to the configuration in the prettierrc but the files they're supposed to format won't get detected.
This makes it so that there is no option but to pass the plugins using the --plugin cli argument.
I think the only sensible way to handle the issue is what has been previously sugested in this thread:
- in the
cli.jsfile we should introspect thepackage.jsonfile the.prettierrcfile for plugin names - Then use
require()to load them.
It's important that the plugin loading step is done in cli.js before the context is loaded because otherwise glob resolution doesn't work correctly
Should this issue title + description also mention pnpm?
Just wondering because some other issues about autoloading Prettier plugins failing in pnpm have been marked as duplicates of this issue.
@karlhorky I think the issue is caused because prettier is not handling PnP correctly but i've only tested it on yarn so I dont know if pnpm has the same problem. If you've got the same sort of problem in pnpm have a look at the fix I suggested in my last comment. If you look at the issue i've linked in that comment you'll see the steps i used and a more thorough explanation of the cause of the problem.
Try following the same steps using pnpm instead of yarn2. If the problem gets fixed for you then we can be sure its caused because of the way prettier handles pnp and it's not an issue specific to yarn or any other package manager.
Check out #11008 - other users have reported similar issues with pnpm, and they have been redirected here as a duplicate. That's why I wrote above that the title + description should maybe include pnpm as well, to make it easier to search for / read through.
Ok so if I'm understanding correctly, the next major version of Prettier (3.0.0) will have the plugin autoload / automatic search feature removed, so that they will need to be specified always.
One of the problems mentioned above is that specifying plugins in the .prettierrc file would load the plugin but it would fail to load / detect the file extensions that that plugin formats. Thus a command like yarn prettier -w . would not run prettier on all the files that the plugins are supposed to format. This is sort of a seperate problem entirely from the plugin autoloading - has it been addressed by the PR #14759?
@Waghabond have you tried require('your-prettier-plugin') instead of 'your-prettier-plugin' in .prettierrc? (example). Without require (or import in Prettier v3), plugins may fail to load in certain package managers. If the problem remains even with require, please create a new issue with reproduction steps.