Skip to content

test: migrate infer-plugins-ext-dir test cases#34

Merged
fabiospampinato merged 3 commits intoprettier:mainfrom
43081j:infer-ped
Jun 14, 2025
Merged

test: migrate infer-plugins-ext-dir test cases#34
fabiospampinato merged 3 commits intoprettier:mainfrom
43081j:infer-ped

Conversation

@43081j
Copy link
Collaborator

@43081j 43081j commented Feb 3, 2025

Pulls the infer-plugins-ext-dir tests from prettier.

{
// TODO (43081j): in prettier, this was `*.foo` and would successfully
// match `src/*.foo`. In prettier CLI, this is not the case
files: ["**/*.foo"],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabiospampinato this one seems like something we should discuss/decide before we merge these tests

it seems prettier treats *.foo as if it was **/*.foo, whereas we treat it as ./*.foo

i think we're correct, but this means a whole bunch of prettier configs may fail with our CLI. maybe we should do a special case that transforms "*.ext" into "**/*.ext"?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think so, no need to break the world here. The problem is that this is a bit tricky, what is the actual logic that we want? Worth checking what v3 is actually doing, because there are many potential ways to implement something like this.

I imagine we should try to explode the glob and do something with that in some cases 🤔

I think we can skip this test for now and fix it later if it's the only blocker for this test suite.

Is everything else basically just copy/pasted?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much, just the paths changed to the mock/fixture extensions. The rest should be the same

@fabiospampinato
Copy link
Collaborator

What's the blocker for this PR, interpreting *.foo as if it was **/*.foo?

@43081j
Copy link
Collaborator Author

43081j commented Feb 23, 2025

it should be good to merge and we can track the *.foo stuff separately

basically globs with no path are treated like **/{glob}. e.g. *.* is **/*.* in prettier. and *.js is **/*.js

@fabiospampinato
Copy link
Collaborator

So in order to select files actually matching *.js one has to write ./*.js?

@fisker
Copy link
Member

fisker commented Feb 23, 2025

I think treating *.js as **/*.foo idea comes from ESLint, but the link https://github.com/prettier/prettier/blob/de30788d30990b35534832ca554f5d5add2d4221/src/config/resolve-config.js#L105 is broken, so I'm not really sure.

@fabiospampinato
Copy link
Collaborator

@fisker presumably it isn't worth changing this, right? 🤔 how it works currently just seems incorrect to me, if I'm understanding it right.

@fabiospampinato fabiospampinato force-pushed the main branch 4 times, most recently from 34e0257 to f8dc396 Compare March 6, 2025 00:45
@fabiospampinato
Copy link
Collaborator

@fisker random question, but if *.js is interpreted as **/*.js, how do we actually pick only the js files in the current directory? is ./*.js the answer?

@fisker
Copy link
Member

fisker commented Mar 6, 2025

I don't understand "pick", are we talking about files in overrides?

@43081j
Copy link
Collaborator Author

43081j commented Mar 6, 2025

Basically I think a non relative wildcard would be assumed to mean ./.

So internally:

  • * becomes ./**/*
  • *.js becomes ./**/*.js
  • foo becomes ./**/foo
  • ./* is left as is
  • **/*.js is left as is (isn't a standalone single *)

That would be like prettier right now I think

@fisker
Copy link
Member

fisker commented Apr 25, 2025

As I said, the files in override logic part is from ESLint, in previous ESLint config system(now it's called "Legacy config") it works the same.

Related code in Prettier core https://github.com/prettier/prettier/blob/0d10f1f846351274922cd205f1d4997206183151/src/config/resolve-config.js#L107

The comment in that file can also confirm the fact, though the link is broken now.

@fabiospampinato
Copy link
Collaborator

If I run the tests just for this I don't see it ever exiting (after a few minutes at least):

NODE_OPTIONS=--experimental-vm-modules npx jest --rootDir test/__tests__ infer-plugins-ext-dir.js --testTimeout 60000

Does this work for you, @43081j?

@43081j
Copy link
Collaborator Author

43081j commented May 14, 2025

mentioned in discord too:

its because the worker exits successfully but worktank doesn't propagate this

that means we continue waiting for the worker even though it exited long ago

under the hood, webworker-shim does emit close events. so worktank should probably listen to those and pass them back to the caller

then inside the CLI, we can decide how to deal with it

@fabiospampinato
Copy link
Collaborator

its because the worker exits successfully but worktank doesn't propagate this

It sounds potentially like a bug on my end, but why is the worker exiting in the first place? 🤔

@43081j
Copy link
Collaborator Author

43081j commented May 14, 2025

its because the worker exits successfully but worktank doesn't propagate this

It sounds potentially like a bug on my end, but why is the worker exiting in the first place? 🤔

because the plugin doesn't exist (which is what the test is testing I assume? I'll have to re-read them to be sure)

either way, its a valid case I think. plugin doesn't exist, so it exits with an error

@fabiospampinato
Copy link
Collaborator

Is it exiting from inside prettier/standalone or on our end? In my mental model prettier/standalone would never just force-exit 🤔 But I guess we should handle that, I think I missed this edge case in worktank, I'll update it 👍

@43081j
Copy link
Collaborator Author

43081j commented May 14, 2025

it is our exit, not prettier's

when you call prettier (via prettier_parallel or serial), we call our own function to load the specified plugins

once we have those, we call prettier core.

but if the load failed, we exit

@fabiospampinato
Copy link
Collaborator

Aaaaah yeah, ok I get it now, I'll fix this in worktank 👍

@fabiospampinato
Copy link
Collaborator

I've just pushed a new version of worktank (v3) that handles workers exiting themselves, among other things. In this case it would throw custom WorkerError that we could check against.

BUT, on a second thought I think it may be cleaner to just not exit where we are exiting, and instead just throwing a custom error, and then checking against that error and exiting then, from the main thread.

43081j and others added 2 commits June 14, 2025 17:33
Pulls the `infer-plugins-ext-dir` tests from prettier.
@fabiospampinato fabiospampinato merged commit a3e797b into prettier:main Jun 14, 2025
4 checks passed
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.

3 participants