-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Update globby
to v11
#6776
Update globby
to v11
#6776
Conversation
with some minor test snapshot updates in tests_integration since fast-glob does not always keep the same file order as node-glob
use of no `nodir: true` option was introduced in 639c523 but it does not seem to be needed with some newer versions of globby
ref: https://github.com/mrmlnc/fast-glob#how-to-write-patterns-on-windows Co-authored-by: fisker Cheung <lionkay@gmail.com> Co-authored-by: Christopher J. Brody <chris@brody.consulting>
This reverts commit f7297cb.
and update some snapshots in tests_integration
# Conflicts: # package.json # yarn.lock
@evilebottnawi I think we can merge this when we got a stable release after this beta version. |
@evilebottnawi No, he didn't. |
Oh, yes, I was wrong mrmlnc/fast-glob#251 |
# Conflicts: # tests_integration/__tests__/__snapshots__/patterns.js.snap
# Conflicts: # package.json
Globby
to v11 (with fast-glob)globby
to v11
aa5893b
to
becc585
Compare
becc585
to
ea7ceb0
Compare
We got the Thanks to @mrmlnc @brodybits |
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.
Good job, but we have bad idea here
// https://github.com/mrmlnc/fast-glob#how-to-write-patterns-on-windows | ||
if (path.sep === "\\") { | ||
patterns = patterns.map(path => path.replace(/\\/g, "/")); | ||
} |
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.
It is bad idea, on linux I can use \
as part of directory or file name
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.
That's exactly why I asked to wrap this replace into if (path.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.
What about WSL? It is allow to use linux filenames and run commands on windows and verse vice. Yes we can try to keep backward compatibility but I think we should throw a warning on using \
as part of glob
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.
No problems with WSL as far as I can see. It behaves just like 'normal' Linux. You can create a file named \
there, but if you look at that file from Windows it won't be named \
. The names are somehow mapped.
Please describe the scenario that you worry about. I don't understand. What exactly can go wrong? Are you simply uncomfortable about the fact that commands with \
aren't cross-platform compatible?
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.
Please describe the scenario that you worry about. I don't understand. What exactly can go wrong?
Glob should use /
always, please read - https://github.com/micromatch/micromatch#backslashes
To solve this, you might be inspired to do something like 'foo\*'.replace(/\/g, '/'), but this causes another, potentially much more serious, problem.
I can spend time investigation in detail why is dangerous, but I also prefer to trust the documentation and not make some mistakes that we are warned about.
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.
Hm, maybe you are right, eslint has same logic https://github.com/eslint/eslint/blob/cf38d0d939b62f3670cdd59f0143fd896fccd771/lib/cli-engine/cli-engine.js#L711
Also look at that https://github.com/eslint/eslint/blob/cf38d0d939b62f3670cdd59f0143fd896fccd771/lib/cli-engine/cli-engine.js#L708
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.
Maybe we should use path.normalize
to allow using ../../foo
too?
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.
It's a horrible mess, but everyone seems to be okay with it. So I guess we should be okay too.
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.
Let's move tests for normalize
to #7587
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.
Also we will add more tests when implement expand directories
, so 👍 on that
JFYI: This problem may affect your package: mrmlnc/fast-glob#253 |
I think we're okay. Our yarn.lock contains only picomatch 2.2.1. The "direct install from GitHub" use case can be broken though. |
docs/
directory)CHANGELOG.unreleased.md
file following the template.✨Try the playground for this PR✨