Consider switching to fast-glob for performance #828
Assignees
Labels
Comments
|
Another related package: sindresorhus/globby (fast-glob under the hood, with a nicer API and good support for async-await down the road). |
|
Looks like there are even better numbers to gain: https://github.com/terkelg/tiny-glob |
|
After some more time and looking around, it looks like it is very popular topic to optimize, those caught my eye especially: https://www.npmjs.com/package/micromatch My use case is very glob-intentive thats why im very interested in this. I can try to do it and make PR if there is a chance of it getting merged. |
|
I tested
Other than that, i must say, results are pretty good. glob
fast-glob
|
Any way to get that working? I think fast-glob documents support for that pattern. |
|
I will try to prepare test case to be sure, maybe i did something wrong (i tested in my app). API looks the same, having the same feature set while providing this kind of performance improvement would be huge. Update You were right, isolated test case has proven that this pattern is supported by fast-glob.
const glob = require('glob');
const fastglob = require('fast-glob'); const pattern = 'node_modules/{glob,fast-glob}/**/*.md'; const globTest = glob.sync(pattern); console.log('glob', globTest); glob [ I ran tests after swapping dependency, 17 failed tests, so i guess it is not plug and play. |
|
Git @nfischer |
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
No change to logic. This updates a test case to create the files it needs inside the temp directory instead of in the repo itself. This is helpful in case the test case fails early, that way we don't leave this file behind. This contributes toward #828, since the change to fast-glob made it clear that this test was mishandling link files and leaving side effects in the git repo.
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
No change to logic. This updates some test cases to create the files they need inside the temp directory instead of in the repo itself. This is helpful in case the test case fails early, that way we don't leave this file behind. This contributes toward #828, since the change to fast-glob made it clear that this test was mishandling link files and leaving side effects in the git repo. However this change is desirably independent of fast-glob.
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
No change to logic. This updates some test cases to create the files they need inside the temp directory instead of in the repo itself. This is helpful in case the test case fails early, that way we don't leave this file behind. This contributes toward #828, since the change to fast-glob made it clear that this test was mishandling link files and leaving side effects in the git repo. However this change is desirably independent of fast-glob.
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
No change to logic. This updates some test cases to create the files they need inside the temp directory instead of in the repo itself. This is helpful in case the test case fails early, that way we don't leave this file behind. This contributes toward #828, since the change to fast-glob made it clear that this test was mishandling link files and leaving side effects in the git repo. However this change is desirably independent of fast-glob.
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
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 (#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue #828.
nfischer
added a commit
that referenced
this issue
Feb 18, 2024
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 #828 Fixes #1149
This was referenced Feb 18, 2024
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
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 (#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue #828.
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds test coverage support for all of the globOptions which share the same name between node-glob and fast-glob. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds tests for more `config.globOptions` values. This aims to cover the options which appear to be supported by both node-glob and fast-glob, however the options have different names in each module. This is a followup to PR #1163. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds tests for more `config.globOptions` values. This aims to cover the options which appear to be supported by both node-glob and fast-glob, however the options have different names in each module. This is a followup to PR #1163. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
No change to logic. This adds tests for more `config.globOptions` values. This aims to cover the options which appear to be supported by both node-glob and fast-glob, however the options have different names in each module. This is a followup to PR #1163. See https://github.com/mrmlnc/fast-glob?tab=readme-ov-file#compatible-with-node-glob Issue #828
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
This deprecates support for configuring `config.globOptions`. Exposing this variable makes it difficult to change (or upgrade) our glob library. This discourages users from depending on this config option going forward. If anyone is using `config.globOptions` then it should continue to function, however this support is not promised for the long-term. 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 (#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue #828.
nfischer
added a commit
that referenced
this issue
Jun 14, 2024
This deprecates support for configuring `config.globOptions`. Exposing this variable makes it difficult to change (or upgrade) our glob library. This discourages users from depending on this config option going forward. If anyone is using `config.globOptions` then it should continue to function, however this support is not promised for the long-term. 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 (#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue #828.
nfischer
added a commit
that referenced
this issue
Jun 21, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 21, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 21, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 23, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 26, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 26, 2024
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 #828 Fixes #1149
nfischer
added a commit
that referenced
this issue
Jun 26, 2024
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 #828 Fixes #1149
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a potential optimization.
I remember we switched to node-glob a while back (I think in v0.6) and got a bit of a performance hit as a result (but with significant gains in correctness/consistency). We might consider switching to fast-glob or one of the related packages to see if we can get performance gains.
This should be an easy code change, and I can help run performance benchmarks. Also, feel free to take a peek at shelljs/benchmarks.
The text was updated successfully, but these errors were encountered: