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
fisker
changed the title
Globby to v11 (with fast-glob)globby to v11
fisker
force-pushed
the
globby-10-update-with-fast-glob
branch
from
aa5893b
to
becc585
Compare
fisker
force-pushed
the
globby-10-update-with-fast-glob
branch
from
becc585
to
ea7ceb0
Compare
|
We got the Thanks to @mrmlnc @brodybits |
| // 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.
It is bad idea, on linux I can use \ as part of directory or file name
There was a problem hiding this comment.
That's exactly why I asked to wrap this replace into if (path.sep === "\\").
There was a problem hiding this comment.
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.
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.
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.
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.
Maybe we should use path.normalize to allow using ../../foo too?
There was a problem hiding this comment.
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.
Let's move tests for normalize to #7587
There was a problem hiding this comment.
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.mdfile following the template.✨Try the playground for this PR✨