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

fix(util): Replace micromatch with picomatch to fix issues with negated globs #11287

Merged
merged 4 commits into from
Apr 12, 2021
Merged

Conversation

danez
Copy link
Contributor

@danez danez commented Apr 12, 2021

Summary

This starts fixing some of the issues mentioned in #9464.

Problem 1:

[
  "**/*.js",
  "!(tests|coverage)/**/*.js
]

This snippet does not ignore anything right now when run through globsToMatcher because the second glob is not considered negated by micormatch/picomatch. Instead it is considered negatedExtglob. Unfortunatelly the scan() API of micromatch does not expose this detail. There is a PR for adding it (micromatch/picomatch#79).

The reason why I completely exchanged micromatch with picomatch is that picomatch has the option to return the parse-state, which makes scan completely unnecessary and shaving off a little bit of work. To visualize it:

// right now
scan(); // simple pre-scan to figure out what features the glob uses
matcher(); // simply forwards to picomatch(glob, {}, returnState: false)

// with this PR
picomatch(glob, {}, returnState: true);

So basically right now scan() scans for features in the glob and then parse has to do the same thing again for parsing. With the PR the state from parsing is used to figure out if the glob is negated.

So to conclude:
There are two options:

  • either adding negatedExtglob to the scan API of micromatch and use this then similar to how it is used in this PR.
  • use picomatch directly which supports negatedExtglob and also makes scan obsolete.

Not sure what you prefer?

Also micromatch was updated to 4.0.4 in devDepedencies because the negated extglob problem was fixed in this version in micromatch() (micromatch/picomatch#78, micromatch/micromatch#213).

Test plan

Added unit tests.

Missing

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

this is awesome, thanks for doing this!

I'm happy to change to using picomatch directly 👍

packages/jest-util/src/globsToMatcher.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Apr 12, 2021

I don't know if you dug into the git history here, but the place you're changing now comes from #10131. If there is some better way than sorta replicating what micromatch does internally than what we landed on there, that'd be awesome 🙂

@danez
Copy link
Contributor Author

danez commented Apr 12, 2021

I don't know if you dug into the git history here, but the place you're changing now comes from #10131. If there is some better way than sorta replicating what micromatch does internally than what we landed on there, that'd be awesome 🙂

I only saw the inline comments but haven't really investigated. From the top of my head, I don't really know if this can be done with micro/picomatch directly. I will keep that in mind and maybe there is something simpler.

Edit: Also the reason for #10131 probably was updating from mm 3 to 4 because 3 had an internal in-memory cache so it would never parse the same glob twice. mm 4 on the other hand has no internal cache and keeps parsing over and over again.

So thinking more about it #10131 might actually have been a partial fix for #9457, but there are still direct calls to micromatch(), so maybe they need to be checked if called multiple times.

@danez danez marked this pull request as ready for review April 12, 2021 16:52
@SimenB
Copy link
Member

SimenB commented Apr 12, 2021

Wonderful, thanks!

Edit: Also the reason for #10131 probably was updating from mm 3 to 4 because 3 had an internal in-memory cache so it would never parse the same glob twice. mm 4 on the other hand has no internal cache and keeps parsing over and over again.

Any chance of re-adding a cache? I guess we could also create some wrapper module around micromatch with a cache (sorta ish what #10131 did, but broader)

},
"devDependencies": {
"@types/graceful-fs": "^4.1.2",
"@types/is-ci": "^2.0.0",
"@types/micromatch": "^4.0.0"
"@types/micromatch": "^4.0.1",
Copy link
Member

@SimenB SimenB Apr 12, 2021

Choose a reason for hiding this comment

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

do we still need this one?

@SimenB SimenB merged commit 9a930df into jestjs:master Apr 12, 2021
@SimenB
Copy link
Member

SimenB commented Apr 12, 2021

Published in 27.0.0-next.8. Thanks again @danez!

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

4 participants