Fix Windows test errors in src/ls.js and test/cp.js. #1165
Conversation
This removes support for configuring `config.globOptions`. Exposing this variable makes it difficult to change (or upgrade) our glob library. It's best to consider our choice of glob library to be an implementation detail. As far as I know, this is not a commonly used option: https://github.com/shelljs/shelljs/issues?q=globOptions currently shows no GitHub issues of users using this option, and there was never really a motivation for why this needed to be exposed (shelljs#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue shelljs#828.
This removes `node-glob` in favor of `fast-glob`. The main motivation for this is because `node-glob` has a security warning and I can't update to `node-glob@9` unless we drop compatibility for node v8. Switching to `fast-glob` seems to be fairly straightforward, although some options need to be changed by default for bash compatibility. Fixes shelljs#828 Fixes shelljs#1149
|
@kmashint can you please rebase this PR on top of the |
a242df1 to
5e83898
Compare
|
@nfischer Will rebase on master tonight. |
5e83898 to
53709e2
Compare
|
I merged master into this pull-request branch, and hoping I cut it the right way, removing references to glob in favour of fast-glob. |
| @@ -246,12 +246,6 @@ function parseOptions(opt, map, errorOptions) { | |||
| } | |||
| exports.parseOptions = parseOptions; | |||
|
|
|||
| function globOptions() { | |||
There was a problem hiding this comment.
I think this was reverted when it should not have been. I think what happened is that you probably have an out-of-date copy of the master branch. Can you please sync this against the main repo's master branch? I think you can do this with something like:
git checkout master
git remote add upstream https://github.com/shelljs/shelljs.git
git fetch upstream
git rebase upstream/master
git checkout switch-fast-glob
git rebase master
git push origin switch-fast-glob --force-with-lease
| @@ -55,7 +55,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "execa": "^1.0.0", | |||
| "glob": "^7.0.0", | |||
| "fast-glob": "^3.3.2", | |||
There was a problem hiding this comment.
Also, I would like to exclude this part from this PR. My change will take care of switching over to fast-glob. I'd like to merge your change before mine though to make sure it is backward-compatible with "plain" glob. Then I'll rebase my change on top of yours to test it with fast-glob`.
It's also fine if you want to test this with fast-glob locally in your checkout. We should just remove this part before merging this on the master branch.
|
Closing this one in favor of #1166. |
These are fixes for Windows in the switch-fast-blob branch as mentioned in this pull-request.
#1153