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

Plugin autoload doesn't work for packages installed from GitHub and for Yarn PnP #8474

Closed
fisker opened this issue Jun 1, 2020 · 28 comments · Fixed by #8465 or #14759
Closed

Plugin autoload doesn't work for packages installed from GitHub and for Yarn PnP #8474

fisker opened this issue Jun 1, 2020 · 28 comments · Fixed by #8465 or #14759
Labels
area:api Issues with Prettier's Application Programming Interface area:plugin api locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:bug Issues identifying ugly output, or a defect in the program

Comments

@fisker
Copy link
Member

fisker commented Jun 1, 2020

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-pug

Expected 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).

@fisker fisker added type:bug Issues identifying ugly output, or a defect in the program area:cli Issues with Prettier's Command Line Interface labels Jun 1, 2020
@thorn0
Copy link
Member

thorn0 commented Jun 1, 2020

Relabeling with "area:api" as this isn't specific to the CLI.

@thorn0 thorn0 added area:api Issues with Prettier's Application Programming Interface and removed area:cli Issues with Prettier's Command Line Interface labels Jun 1, 2020
@fisker fisker mentioned this issue Jun 1, 2020
4 tasks
@fisker
Copy link
Member Author

fisker commented Jun 1, 2020

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?

@thorn0
Copy link
Member

thorn0 commented Jun 1, 2020

Checking package.json to see what plugins are installed might work.

@fisker
Copy link
Member Author

fisker commented Jun 1, 2020

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.

@fisker
Copy link
Member Author

fisker commented Jun 1, 2020

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"
    );

@thorn0
Copy link
Member

thorn0 commented Jun 1, 2020

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?

@fisker
Copy link
Member Author

fisker commented Jun 1, 2020

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.

@thorn0
Copy link
Member

thorn0 commented Jun 1, 2020

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?

@fisker
Copy link
Member Author

fisker commented Jun 1, 2020

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

@fisker fisker mentioned this issue Jun 2, 2020
4 tasks
@thorn0
Copy link
Member

thorn0 commented Jun 4, 2020

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.

@alexander-akait
Copy link
Member

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.json to get list of plugins (they should be prettier-plugin-*/@prettier/plugin-*).
  • Using require to load them.
  • Implement --plugin option for non package.json environments and for projects which doesn't have package.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-name in dependencies, now we load a plugin, after this we can't load. But we can keep globbing from node_modules to 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.

@thorn0
Copy link
Member

thorn0 commented Jun 4, 2020

In this case I think we should implement --plugin flag for CLI

We have it. https://prettier.io/docs/en/plugins.html#using-plugins

@alexander-akait
Copy link
Member

@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.

@thorn0
Copy link
Member

thorn0 commented Jun 4, 2020

@evilebottnawi It supports that. It accepts what require accepts.

@fisker
Copy link
Member Author

fisker commented Jul 21, 2020

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

  1. plugins in package.json
  2. plugins in .prettierrc (No support yet)

@kachkaev
Copy link
Member

kachkaev commented Oct 18, 2020

Side suggestion: adding prettierrcplugins field: #7073 (comment)

Happy to create a new issue / PR if there are no major flaws in the concept.

@fisker
Copy link
Member Author

fisker commented Oct 19, 2020

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.

@kachkaev
Copy link
Member

kachkaev commented Oct 19, 2020

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()?).

@fisker
Copy link
Member Author

fisker commented Oct 19, 2020

Is it necessary that the resulting Prettier config is serializable?

Personally I use .js , but I feel there will be objections.

@kachkaev
Copy link
Member

It’s already possible to declare configplugins and that works with Yarn PnP 😳 🎉

👀 #7073 (comment)

@Waghabond
Copy link

It’s already possible to declare configplugins and 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.js file we should introspect the package.json file the .prettierrc file 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

@karlhorky
Copy link
Contributor

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.

@Waghabond
Copy link

@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.

@karlhorky
Copy link
Contributor

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.

@karlhorky
Copy link
Contributor

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.

@Waghabond
Copy link

Waghabond commented May 5, 2023

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?

@kachkaev
Copy link
Member

kachkaev commented May 5, 2023

@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.

@Waghabond
Copy link

@kachkaev yeah i have, i actually made a bug report explicitly about the issue that the plugins get loaded but their file extensions dont get detected if load them using require() in the .prettierrc #13276

@github-actions github-actions bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Nov 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area:api Issues with Prettier's Application Programming Interface area:plugin api locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. type:bug Issues identifying ugly output, or a defect in the program
Projects
None yet
6 participants