Fix plugin API in globally installed Prettier and introduce optional --plugin-search-dir#4192
Fix plugin API in globally installed Prettier and introduce optional --plugin-search-dir#4192ikatyang merged 9 commits intoprettier:masterfrom kachkaev:fix-global-plugin-api
Conversation
|
Does this only look in the first node_modules dir upwards? Why not go up forever? Yarn workspaces and differences between npm and yarn are some things to consider here. |
|
Do you think we should search for all There is one extra bit that I've added a bit later – see 61d87af. A directory with node modules is searched in two steps now. The first one succeeds for standard prettier instances (that is both local and global dependencies), while the second step is useful for the |
|
I fixed one typo and the tests started to fail. Looks like we need a better algorithm to decide how to find parent
works for
does not work for
works for
does not work for
|
|
Perhaps, if we add some special env variable for tests, that'd be it, however, this option smells. In any case, we should not trust cwd as it used to be because of #4000 (comment):
|
const nodeModulesDir =
findDirUp(process.env.PRETTIER_PLUGINS_ROOT, "node_modules") ||
findDirUp(__dirname, "node_modules") ||
findDirUp(process.cwd(), "node_modules");🤔 or even const nodeModulesDir =
findNodeModulesDir(process.env.PRETTIER_PLUGINS_ROOT) ||
findNodeModulesDir(__dirname) ||
findNodeModulesDir(process.cwd());Assuming tests set |
|
Or how about
If |
|
This PR is updated with a new
|
kachkaev
changed the title
kachkaev
changed the title
|
Can you really call plugins "auto"-discoverable if you have to specify an option pointing to where they are? 😅 As an aside, Could we combine pkg-search-up with |
|
Yeah, I also see
I'm afraid that's not the way to go, because we don't want ambiguity caused by multiple plugin installs on one machine. Should a local instance of prettier fall back to global yarn or npm plugins if none are found locally? And what about if some are found? Guess the answer is no for both cases, unless there is guidance from a user (i.e. a custom-defined In future, we can support multiple |
|
I think I misunderstood the goals and constraints of the PR. Disregard my comments until I have more time to look into this properly |
|
You probably thought that |
|
Spotted a bug in When there exists findDirUp(__dirname, "node_modules")with findDirUp(path.resolve(findDirUp(__dirname, "prettier"), ".."), "node_modules")I'm not sure how to cover that with tests though. |
|
Not a huge fan of the I'd be interested to see if the |
kachkaev
changed the title
|
Hi @azz 👋 I thought about setting npm install --global prettier @prettier/plugin-a prettier-plugin-b prettier-plugin-cAnd then enjoy prettier "**/*.{js,md,a,b,c}"in any folder on their machine. I know that having local dependencies is better, but despite that a global instance of Prettier powered by several plugins can be very useful, say, in editor extensions such as Thus, we are left with searching for actual packages above the tree from where Prettier is and, to be honest, this is not way more complex: const nodeModulesDir = findDirUp(
path.resolve(findDirUp(__dirname, "prettier"), ".."),
"node_modules"
);
const autoPluginNames = findPluginsInNodeModules(nodeModulesDir);(we first find parent Some complexity comes into play indeed, but only if we want to override this behaviour for two very rare edge cases:
const nodeModulesDir = pluginSearchDir
? path.resolve(process.cwd(), pluginSearchDir, "node_modules") // <--------
: findDirUp(
path.resolve(findDirUp(__dirname, "prettier"), ".."),
"node_modules"
);This is only needed in advanced uses, mostly while debugging Prettier. Everyone else simply enjoys autoloaded plugins no matter if the instance is global and what the current I'm open to alternative approaches that would fix #4000 and I don't insist that my implementation needs to be merged into prettier. However, after several days of thinking on this problem I can't come up with any other solution that would satisfy all the constraints that the problem of plugin autodiscovery has. |
|
Technically we can remove However, without |
|
@suchipi when do you think you could look at this? Curious to hear your opinion! Also ping @evilebottnawi @ikatyang @j-f1 🙌 |
|
@kachkaev I haven't had a chance to look at this yet but I haven't forgotten about it. I found somewhere that you linked to prettier/prettier-atom#395 which helps me understand the context of and motivation for this change better. My main concerns are:
Some "nice-to-have"s are that these would both work: yarn global add prettier
yarn global add @prettier/plugin-php
prettier --write './**/*.php'npm i -g prettier
npm i -g @prettier/plugin-php
prettier --write './**/*.php'My kneejerk reaction to I'll try to give this a thorough review sometime soon, but feel free to merge this without a review from me if it's ready. |
|
I squashed all commits in this PR and rebased them on top of the latest master. Tests seem to pass. Original unsquashed commits are now in @suchipi when I declare that It'd be great if someone could look at this PR at some point – we need to fix plugins in a global prettier setup 💯 |
|
Is there still interest in this PR? I'm really keen to see plugins working in a global instance of Prettier. Please help me to help! 🙂 It'd be great if I could have some design feedback and work on conflict resolution if all is OK. |
|
Hi @kachkaev, I have had this PR on my mind and I still have interest in it but haven't looked at it in a while because I've been pretty busy. I cannot promise when I will be able to look at this but I want prettier plugins to work with global installations, too. |
|
I apologize that this PR has been open for so long 🙏 |
|
I guess I'll probably wait for #4341 to land to master before rebasing this PR. That's because |
…--plugin-search-dir
kachkaev
changed the title
|
@ikatyang I'm using (I also built Please let me know if you have any questions about autoloading, it's not the most straightforward thing to figure out. What do you guys think in general? What else is left to do? |
…r" does not work due to rollup
| const supportInfo = support.getSupportInfo(null, { | ||
| plugins: options.plugins | ||
| plugins: options.plugins, | ||
| pluginSearchDirs: options.pluginSearchDirs |
There was a problem hiding this comment.
The options.plugins here should be already loaded, no need to specify pluginSearchDirs.
|
|
||
| const supportOptions = getSupportInfo(null, { | ||
| plugins: options.plugins, | ||
| pluginSearchDirs: options.pluginSearchDirs, |
There was a problem hiding this comment.
Indeed – removing pluginSearchDirs: options.pluginSearchDirs in both cases did not break local tests. This must be an artefact of my earlier attempts prior to #4364.
|
|
||
| describe("automatically loads 'prettier-plugin-*' from package.json devDependencies", () => { | ||
| runPrettier("plugins/automatic", ["file.txt", "--parser=foo"]).test({ | ||
| if (process.env.NODE_ENV !== "production") { |
There was a problem hiding this comment.
We have a third-party.js that is used for mocking this kind of things:
prettier/src/common/third-party.js
Lines 3 to 9 in 3fe790d
for example:
prettier/tests_integration/runPrettier.js
Lines 71 to 76 in 3fe790d
|
|
||
| const pluginPath = resolve.sync(plugin, { basedir: process.cwd() }); | ||
| return Object.assign({ name: plugin }, eval("require")(pluginPath)); | ||
| const externalPluginInfos = []; |
There was a problem hiding this comment.
It'd be better to not mutate array if possible as it's clearer, I'd prefer something like this:
const externalManualLoadPluginInfos = plugins.map(pluginName => ({
name: pluginName,
requirePath: resolve.sync(pluginName, { basedir: process.cwd() })
}));
const externalAutoLoadPluginInfos = pluginSearchDirs
.map(pluginSearchDir => {
const resolvedPluginSearchDir = path.resolve(
process.cwd(),
pluginSearchDir
);
if (!isDirectory(pluginSearchDir)) {
throw new Error(
`${pluginSearchDir} does not exist or is not a directory`
);
}
const nodeModulesDir = path.resolve(
resolvedPluginSearchDir,
"node_modules"
);
return findPluginsInNodeModules(nodeModulesDir).map(pluginName => ({
name: pluginName,
requirePath: resolve.sync(pluginName, {
basedir: resolvedPluginSearchDir
})
}));
})
.reduce((a, b) => a.concat(b), []);
const externalPlugins = uniqBy(
externalManualLoadPluginInfos.concat(externalAutoLoadPluginInfos),
"requirePath"
).map(externalPluginInfo =>
Object.assign(
{ name: externalPluginInfo.name },
eval("require")(externalPluginInfo.requirePath)
)
);| @@ -1,22 +1,108 @@ | |||
| "use strict"; | |||
There was a problem hiding this comment.
It seems the current implementation does not support pluginSearchDirs in config files, which we should do something like the following in resolve-config.js:
options.pluginSearchDirs = options.pluginSearchDirs.map(
pluginSearchDir => path.resolve(
path.dirname(configFilePath),
pluginSearchDir
)
)
|
Many thanks for your review comments @ikatyang, especially for a trick with mocking! It took me ages to figure out what how to test One thing I did not address is supporting How about starting a separate discussion in a new PR and too keep this one concise? |
| const globby = require("globby"); | ||
| const path = require("path"); | ||
| const resolve = require("resolve"); | ||
| const thirdParty = require("../common/third-party"); |
There was a problem hiding this comment.
const thirdParty = require("./third-party");| // We cannot use `jest.setMock("get-stream", impl)` here, because in the | ||
| // production build everything is bundled into one file so there is no | ||
| // "get-stream" module to mock. | ||
| jest.spyOn(require(thirdParty), "getStream").mockImplementation(() => ({ |
There was a problem hiding this comment.
Any reason you want to move it? IMO, we shouldn't touch unrelated code.
There was a problem hiding this comment.
The reason for this change was that I wanted to sort third-party scripts alphabetically. I reverted that in bfbc580, but I'd be keen to delete it and thus to keep sorting. This will help read and write other third-party scripts in future. WDYT?
| When plugins cannot be found automatically, you can load them with: | ||
|
|
||
| * The [CLI](./cli.md), via the `--plugin` flag: | ||
| * The [CLI](./cli.md), via the `--plugin` and `--plugin-search-dir`: |
There was a problem hiding this comment.
We don't need to pass --plugin and --plugin-search-dir simultaneously, these two options should be documented separately for clarity, or at least examples should be separated.
There was a problem hiding this comment.
I'm not sure why someone can't use --plugin and --plugin-search-dir simultaneously – that's technically possible and can be useful when debugging how a bunch of plugins work together (e.g. we have python and php plugins installed globally and we're testing if a locally developed elm plugin with a locally installed prettier does not conflict with the other two). Seeing both options together looks logical to me after this phrase in the docs:
When plugins cannot be found automatically, you can load them with:
I don't feel there is much distinction between --plugin and --plugin-search-dir for a user, so I don't see much value in splitting the narrative into two separate sections. The only potential benefit that I see is consistency with how other CLI options are documented 🤔
Shall we keep the docs as is until the plugin ecosystem graduates from beta and thus deserves extra attention anyway?
There was a problem hiding this comment.
I originally meant they could be used simultaneously but not required since they are completely different and won't affect each other.
After second thought, the examples are relative paths so people should understand that they're separated. This PR should be good to go now.
Sure. And currently the options from |
|
Sure. |
|
Opened #4446 to track relative paths for |
|
🎉 Thanks for working through this problem so diligently, @kachkaev! |
lock
Bot
added
the
locked-due-to-inactivity
5 participants
Closes #4000 🎉
Instead of searching for
dependenciesanddevDependnencies, in the nearestpackage.json, Prettier now searches for["prettier-plugin-*/package.json", "@prettier/plugin-*/package.json"]in the nearest parentnode_modulesdirectory. This makes it possible for a globally installed instance to find plugins despite the lack ofpackage.jsonup the tree. What is also good about this approach is that plugins are discoverable when they are a part of some boilerplate packages likecreate-react-app. Their philosophy is to stuff a project with all bells and whistles at a cost of just a single dependency inpackage.json.Rather than matching directories like
prettier-plugin-*and@prettier/plugin-*insidenode_modules,loadPlugins()is looking forprettier-plugin-*/package.jsonand@prettier/plugin-*/package.json. I decided to do this for two reasons::globby, which has already been in Prettier dependencies (so no need to reinvent the wheel)package.jsonfile is more robust – there'll be no false-positives on empty dirsThis change in Prettier logic has been approved by @azz, the author of #3536 and #3624.
UPD: After a few iterations on this PR, I also added an optional
--plugin-search-dirCLI parameter, justification of which you will find below.