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

feat: Allow random order of globs #11293

Closed
wants to merge 1 commit into from
Closed

feat: Allow random order of globs #11293

wants to merge 1 commit into from

Conversation

danez
Copy link
Contributor

@danez danez commented Apr 13, 2021

BREAKING CHANGE: This changes the behavior of all config options with arrays of globs to not respect the order similar to how it was in jest 24 and lower.

Fixes #9464 (one or the other way)

Summary

Some history: micromatch 3 (jest 24 and lower) allowed random order of globs (like ['!foo.js', 'foo.js'] would ignore foo.js), whereas micromatch 4 changed this and respects the order (['!foo.js', 'foo.js'] would match foo.js because the second glob runs after the first)
Also negations work slightly different, for example ['*/**.js', '!(test)/**/*.js'] in mm 3 would match foo.js, in mm 4 it would not. Both versions are kinda correct because the negated glob alone only matches "any js file in at least one subfolder that is not named test", because of !(test)/ (this is also that way in globs in bash), but combined with the first (positive) glob the behavior is outside of the "specified" scope of globbing and more like up to the libs how the combine and merge multiple globs.

Both of these changes also happened in jest in version 25 when micromatch was updated to version 4. (e.g. collectCoverageFrom).

Unfortunately, there is no right or wrong here and it's more based on opinion, so treat this PR more like a discussion for now.

So either jest keeps the way it does globs right now (ordered globs) which allows things like:

// Match all js files in all subfolders, 
// but ignore all js files in subfolders in the folder `test` and `coverage`, 
// except for the js files in `test/utils`
[
  '**/*.js',
  '!(test|coverage)/**/*.js',
  'test/utils/*.js'
]

To accomplish the same with this PR:

[
  '**/*.js',
  '!coverage/**/*.js',
  '!test/!(utils)/**/*.js'
]

Or switch back to the old behavior (jest 24 or lower) where one can do:

// Order does not matter, once a file is ignored it stays ignored, even if it would match in a subsequent glob
[
  '!(test|coverage)/**/*.js',
  '**/*.js'
]

Oh and the same can independently be decided for the negated (mis)behavior, although that could actually be considered broken and be fixed, as hardly anyone will be able to take advantage of the current feature. (I will bring this also up in mm/pm itself)

Test plan

Adjusted unit tests and added new unit tests

TODO

  • Discuss and find a preferred way
  • Changelog for this PR
  • Update docs for the config options with multiple globs and specify the behaviour the globs have.

BREAKING CHANGE: This changes the behavior of all config options ot not respect the order of the globs similar to how it was in jest 24 and lower.
const isMatch = picomatch(glob, picomatchOptions, true);
const negated = glob.startsWith('!');
const isMatch = picomatch(
negated ? glob.slice(1) : glob,
Copy link
Contributor Author

@danez danez Apr 13, 2021

Choose a reason for hiding this comment

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

This glob.slice(1) is the fix for the "wrong" negation behavior mentioned in the main PR text.
By removing the exclamation mark we do match the ignored path but later know because of the negated flag that a match means ignore this file.

@SimenB
Copy link
Member

SimenB commented Apr 14, 2021

This is very interesting, thanks for putting it together along with a thorough explanation!

I have to say I find micromatch@4's behaviour more intuitive/easier to understand. Based on that, the fix in #9464 (for the people who saw an improvement rolling back micromatch) is to change the order of their globs?

/cc @cpojer @jeysal @thymikee thoughts?


As a side note, this change in behavior between v3 and v4 should IMO be mentioned in the changelog: https://github.com/micromatch/micromatch/blob/5318752abc2f33153b3ccddf9f1f3b7682000a43/CHANGELOG.md#400---2019-03-20

@jeysal
Copy link
Contributor

jeysal commented Apr 14, 2021

I agree that on the ordering, micromatch 4 makes more sense (and seems strictly more powerful?), I'd like to be able to specify exceptions to a negated glob later on.
The example on how negated globs work slightly differently I must admit I didn't understand, maybe I'm missing knowledge on how the parentheses or something work in general.

@cpojer
Copy link
Member

cpojer commented Apr 15, 2021

I agree, I would keep the current behavior.

@danez
Copy link
Contributor Author

danez commented Apr 15, 2021

Okay will create PR for the docs and describe how globs are applied.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Jest 25's change in behaviour of collectCoverageFrom option
5 participants