Don't auto-load prettier-plugin-organize-imports in tests

[lehni]

I noticed this during debugging #419:

The prettier-plugin-organize-imports plugin gets auto-loaded in the unit tests, and it has side-effects that effect the parsers. To make sure we really only test this plugin and exclude such side-effects, we should pass pluginSearchDirs: false to the formatOptions by default.

[Shinigami92]

Could you add a test that would fail if we would not set that?

[lehni]

@Shinigami92 I'd rather not because I don't want to rely on side-effects in other plugins, and I don't have the time to write a plugin just to test for such a simple setting. We should just only load our own plugin in the tests, and this setting does that.

[Shinigami92]

@lehni But this setting is just set now in the tests, but before you set it, everything was working the same as before (all tests were green as after the change)
So I don't know / understand why this setting is really needed.

Beside that, I thought setting plugins: [plugin] would result in pluginSearchDirs: false automatically, but correct me if I'm wrong.

I would still love to see a test that is somehow affected by this change, so we can see that it has somehow a real benefit.

[lehni]

Setting plugins: [plugin] doesn't set pluginSearchDirs: false, it just adds another plugin to load. We should set pluginSearchDirs: false because we don't want other plugins from node_modules to be loaded, it's as simple as that. It doesn't affect the current tests, but it did affect that warning that I was receiving in #419, and like outlined above prettier-plugin-organize-imports in the depencencies of this plugin was somehow leading to that warning not occuring, while in my own codebase where I'm using plugin-pug, the warning does appear. I don't understand how prettier-plugin-organize-imports is causing this different behavior, but it is doing it.

Setting pluginSearchDirs: false is the right thing to do here, and this setting doesn't need a test.

End of story for me. I don't have the time to write a test for it, and it's time ill spent, because such a test isn't needed.

[Shinigami92]

but it did affect that warning

If that's the case, then that is what we need to test!
That can be done via a vitest spy on console.warn

[lehni]

See here:

It seem weird to write a test that wrongly checks a vue directive using the angular parser, but that's what's happening when pugFramework ends up with the 'auto' value.

That's the only situation where the above became an issue. I just created this PR to avoid such future situations but do what you want with it. Writing a test for it, I won't.

[lehni]

One last thing:

• options.plugins corresponds to the --plugin CLI option: https://prettier.io/docs/en/plugins.html#using-plugins
• options.pluginSearchDirs = false corresponds to the --no-plugin-search CLI option: https://prettier.io/docs/en/cli.html#--no-plugin-search

[fisker]

Actually, this make sense, we do the same in core prettier/prettier#12322, this is also hard to test unless we relay on behavior of existing plugin.

But I don't think this has something to do with #419, like I commentted, check options.parentParser is the right way to fix #419

[lehni]

@fisker it does, see here: #419 (comment)

[Shinigami92]

@lehni I now added a test for the console.warn, please have a look
But I might not have fully understand your comment #419 (comment)
I cannot observe any difference when I comment in and out the introduced line in tests/common.ts. So I'm still unsure what the benefit is, until I have a test that fails especially when pluginSearchDirs is set to false.
Maybe we need to load prettier-plugin-organize-imports explicitly into the plugins-option in a specific test written for that 🤔
Or is your environment different to mine? At the end it should fail on GH Actions...

[lehni]

@Shinigami92 this is the strangest thing… I also cannot reproduce this problem anymore. While debugging this situation 3 days ago, I have done some manual edits in source files from dependencies, mostly to add console logs and such. I also ran prettier directly using the CLI, and passing in various --plugin related arguments. And after a whole lot of digging, I was unable to get the tests to spit out this warning, it only happened when I ran the CLI. So I went digging and noticed that the tests were loading the prettier-plugin-organize-imports plugin, and that if I turned off the loading, the warning appeared.

With a fresh instal of the repo, it now seems that this plugin isn't loaded either way. I would still pass this setting for the reason that @fisker has said, but leave it up to you. The tests seems fine otherwise, but a simpler and more abstract markup would also do, e.g.

.container(v-directive="{ property: !!value }")

[lehni]

@fisker I understand that you think that options.parentParser should be used, but I know nowhere near enough about all this to understand where to even start… And as pointed out before, when setting pugFramework: 'vue', the warning goes away, so I am not sure this is the solution. It works for me now and I don't have time to work more on this plugin, so I'll have to leave it at this. Also, my comment above that you thumbed down was simply to point out that the mentioned plugin does indeed have side-effects. But you're of course right, this has nothing to do with the actual issue, just with the sudden disappearance of the warning that I had experienced.

[Shinigami92]

@Shinigami92 this is the strangest thing… I also cannot reproduce this problem anymore. While debugging this situation 3 days ago, I have done some manual edits in source files from dependencies, mostly to add console logs and such. I also ran prettier directly using the CLI, and passing in various --plugin related arguments. And after a whole lot of digging, I was unable to get the tests to spit out this warning, it only happened when I ran the CLI. So I went digging and noticed that the tests were loading the prettier-plugin-organize-imports plugin, and that if I turned off the loading, the warning appeared.

With a fresh instal of the repo, it now seems that this plugin isn't loaded either way. I would still pass this setting for the reason that @fisker has said, but leave it up to you. The tests seems fine otherwise, but a simpler and more abstract markup would also do, e.g.

.container(v-directive="{ property: !!value }")

[lehni]

I guess we can close this?