-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore: upgrade to micromatch 3 #6650
Conversation
Thanks for the PR! Mind adding a test as well? |
We have a helper doing something similar in |
@SimenB there is already tests that was able to catch this problem, but they are disabled on windows @thymikee ye, i saw, and there also similar thing in jest-regex-util, but not exactly same. |
@aldarund feel free to wrap this test single test in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, thank you so much or tackling it!
Mind adding an entry to the changelog?
Might be a good idea to rebase first, as you'll probably get a conflict
Codecov Report
@@ Coverage Diff @@
## master #6650 +/- ##
==========================================
- Coverage 68.28% 68.27% -0.01%
==========================================
Files 249 250 +1
Lines 9628 9633 +5
Branches 5 6 +1
==========================================
+ Hits 6574 6577 +3
- Misses 3052 3054 +2
Partials 2 2
Continue to review full report at Codecov.
|
@SimenB done |
@mjesun this might be worth a patch release |
…-find-tests # Conflicts: # CHANGELOG.md
We've switched back to micromatch 2. Is this still necessary? |
Not sure about the last commit, although it seems correct: https://www.npmjs.com/package/micromatch#backslashes |
Finally green. The normalization is super ugly, but I think we should clean that up in jest 25 with #7185 The issue here is that globs need forward slash as separator, which doesn't work for regexes (at least not how we use them currently). I think it'll be easier to normalize correctly when it's all globs (or we'll at least be forced to go through where it's used and fix it properly) |
Agreed. Btw, how about placing |
Looks like we're pretty consistent now in terms of escaping globs for micromatch. Would be even better to extract a helper for matching globs that uses micromatch as its implementation detail, which seems like a nice followup refactor to this PR :) |
I can try to PR |
Would be awesome if they agreed! Maybe it's better to ask first |
🤞 hopefully it'll work out 🙂 |
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. |
Summary
Fixes #6546 .
From micromatch docs ->
Test plan
Same config as in referenced issue work fine with this fix