-
-
Notifications
You must be signed in to change notification settings - Fork 130
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
Upgrade fast-glob
package to v3
#126
Conversation
fast-glob v3 does no longer convert slashes in glob pattern.
This comment has been minimized.
This comment has been minimized.
index.js
Outdated
@@ -89,14 +90,16 @@ const getFilterSync = options => { | |||
DEFAULT_FILTER; | |||
}; | |||
|
|||
const normalizeGlob = glob => glob.split(path.sep).join(path.posix.sep); |
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.
Note:
Only forward-slashes in glob expression. Previously, we convert all slashes to the forward-slashes, which did not allow the use of escaping. See pattern syntax section in the README.md file. - https://github.com/mrmlnc/fast-glob/releases/tag/3.0.0
I don't think we should override that as the change was done for a good reason. Instead, we should update the tests and docs.
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.
I agree. An only concern at here is the glob pattern generated by dir-glob
. It includes unexpected backslashes because of using path.join
in Windows. (kevva/dir-glob#17)
New release of dir-glob is out: https://github.com/kevva/dir-glob/releases/tag/v3.0.0 Can you bump it here? |
Hm, a known failure is pass only in Windows. Lines 188 to 195 in 8aadde8
|
Remove the |
It is still failing in Linux and macOS. I looked into the cause a little bit but I had no idea 🤔 |
Hi guys! When I released fast-glob, I also tested its serviceability in the globby package with some patch: Maybe this will help in some way. But it was monkey-patching 🐒 |
Hmmmm… looks strange for me. Output for Line 131 in 8aadde8
Windows (PowerShell):
Ubuntu (WSL):
Most likely this is related to the |
Because I just worked to fix generated patterns for Windows to be compatible with an updated micromatch, I did not exactly intent to resolve a known failing test or change existed behavior in globby by merging kevva/dir-glob#18. It may take some time but I'm going to continue to debug... |
Found out a reason!: kevva/dir-glob#18 (comment) We can upgrade fast-glob by merging kevva/dir-glob#19 to dir-glob. |
fast-glob
package to v3.0.2fast-glob
package to v3.0.3
Thanks a lot 🙏 Bumped dir-glob and fast-glob. |
fast-glob
package to v3.0.3fast-glob
package to v3
Released: https://github.com/sindresorhus/globby/releases/tag/v10.0.0 Thanks for the help 🙌 |
fast-glob v3 has an improved performance and some bug fixes from micromatch upstream package.
In Node 10.16.0 on my Mac,
npm run bench
would be faster 3 to 4 times thanmaster
.I was received a report to my tool, about a wrong globbing by globby (marp-team/marp-cli#108). It's a bug from the upstream package so would be resolved by this.