Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.31%. Comparing base(d9d7f4d) to head(c63bce9).
Report is 2 commits behind head on master.

@@            Coverage Diff             @@##           master    #1153      +/-   ##==========================================+ Coverage   97.28%   97.31%   +0.03%==========================================  Files          36       36                Lines        1364     1380      +16     ==========================================+ Hits         1327     1343      +16  Misses         37       37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report?Share it here.

Hello! I was wondering if you are planning to merge this. There now also deprecation warnings being displayed because theinflight package is marked as deprecated.

As a possible data point regarding the safety of this switch, I tried running theeslint test suite using this branch forshelljs, and it worked without problems.

I'm also interested in this fix to avoid potential global and inflight issues.

Hey, thanks for the interest. It's good to hear that this is important to other folks in the community.

This is going to be tricky for me to work on. I don't have a windows machine anymore so it's going to be difficult for me to get the tests passing on Windows. I know that a lot of projects only use ShellJS on Mac or Linux, however Windows is still critical for some users. If anyone has experience developing on Windows and is able to give this branch a try, I'd love to collaborate on this together.

That makes sense! I also don't currently have a Windows machine, but am a bit curious and might be able to take a look together with a friend who has one. (Not sure when though, and while I do have some Windows development experience it was a long time ago, so if anyone else who is able to help comes along that might be preferable :) )

I have Windows 10, so I'll give try the branch tonight.

From a first pass on Windows 10 these look like platform-dependent differences. Maybe they should be skipped for Windows. I'll check again in some more detail tomorrow.

npm run test...  557 passed  12 tests failed  6 skippedWarning: skipping platform-dependent test 'symbolic mode'  ls » recursive, no path  D:\_Project\shelljs\test\resources\ls\test\ls.js:281  ls » recursive, path given  D:\_Project\shelljs\test\ls.js:291  ls » -RA flag, path given  D:\_Project\shelljs\test\ls.js:301  ls » -RA flag, symlinks are not followed  D:\_Project\shelljs\test\ls.js:312  ls » long option, directory, recursive (and windows converts slashes)  D:\_Project\shelljs\test\ls.js:464  ls » non-normalized paths are still ok with -R  D:\_Project\shelljs\test\ls.js:531  find » current path  D:\_Project\shelljs\test\test\find.js:31  find » simple path  D:\_Project\shelljs\test\find.js:41  find » multiple paths - comma  D:\_Project\shelljs\test\find.js:50  find » multiple paths - array  D:\_Project\shelljs\test\find.js:59  find » -L flag, folder is symlinked  D:\_Project\shelljs\test\find.js:74  cp » cp -p should preserve mode, ownership, and timestamp (directory)  D:\_Project\shelljs\test\cp.js:876

I've created fixes for Windows with this pull-request, thanks!npm run test is clean on Windows 10.
#1165

Someone pointed out that Issue#1148 is actually caused by our dependency on the glob library. Switching tofast-glob will almost certainly fix that issue too.

@nfischer I'll close this messy one and opened a simpler one here against shelljs/shelljs:master. Hmm, maybe I can't close it myself.
#1166

PR#1166 was a huge help. Thanks@kmashint!

Still some more work to be done before this is mergeable. I plan to take another look to see if I can fix up the last test case.

This seems like a breaking change since the syntax forconfig.globOptions has changed. If you're going to be making a breaking change anyway, maybe drop support for Node 8?

fast-globhas 17 dependencies.fdir is pretty nice and doesn't have any. Here's an example of switching fromglob tofdir in case you want to consider it:rollup/plugins#1741

This seems like a breaking change since the syntax forconfig.globOptions has changed.

This PR includes backward compat for some (most?) of the oldglobOptions. Let me know if you still feel this is a breaking change—I'm open to feedback on this, but I just want to make sure you're aware of the backward compat code.

If people are motivated to help with backward compat, I would appreciate support in writing test cases (example PR#1164) and writing compat code.

If you're going to be making a breaking change anyway, maybe drop support for Node 8?

Agree that dropping makes sense if we do a breaking change release. I'm holding off on merging until I can decide on the right path to release this (e.g., the choice would be between releasing as 0.9.0 or as 0.8.6). I appreciate your patience while I think through this.

fast-globhas 17 dependencies.fdir is pretty nice and doesn't have any. Here's an example of switching fromglob tofdir in case you want to consider it:rollup/plugins#1741

Do you have a specific concern with the dependency tree?

This PR includes backward compat for some (most?) of the old globOptions. Let me know if you still feel this is a breaking change—I'm open to feedback on this, but I just want to make sure you're aware of the backward compat code.

Ah, thanks. I'll admit I just read the change to docs and missed the compat layer. It'd probably be worth a mention in the docs that the old options still work though are discouraged. It could also print some kind of deprecation warning to help users migrate to the new options. Or maybe that's unnecessary already being done at a higher level as I see the options in general are now deprecated since the last release. I would say the compat layer feels quite tricky to make 100% compatible and I wouldn't be surprised if there are edge cases

Do you have a specific concern with the dependency tree?

fast-glob just seems larger than it needs to be. It has 17 dependencies with a total of 12 maintainers whilefdir has 1. That opens you up to 12x as much supply chain risk and 17x as much risk of users getting hit by deprecation warnings. It's also 8.5 times as large on disk - though that's probably less of a concern given that disk space is cheap.fast-glob andfdir both includepicomatch so the underlying implementation is very similar.

Someone pointed out that Issue#1148 is actually caused by our dependency on the glob library. Switching tofast-glob will almost certainly fix that issue too.

Just to clarify that#1148 would also be resolved by upgrading to a supported version of glob. If this isn't going in soon, would you take a PR just to upgrade glob?

We landed here because of Snyk security vulnerabilities as well. I'd be glad to help with some guidance to get this PR through!

I still intend to get this change landed, however I'm going to give a brief update that I do not consider this to be a security issue. Please see my response on#1149 (comment). In short, shelljs never calls theinflight library because we only useglob.sync(), not the async-glob. Therefore shelljs is not vulnerable to the inflight issue.

There is now a new library calledtinyglobby available, which is a drop-in replacement forfast-glob that might be of interest. It's been quickly adopted by lots of prominent projects like vitest and nuxt and has a much smaller footprint thanfast-glob

any luck on merging this branch?