Skip to content
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

Upgrade fast-glob package to v3 #126

Merged
merged 7 commits into from Jun 29, 2019

Conversation

yhatt
Copy link
Contributor

@yhatt yhatt commented Jun 25, 2019

fast-glob v3 has an improved performance and some bug fixes from micromatch upstream package.

In Node 10.16.0 on my Mac, npm run bench would be faster 3 to 4 times than master.

$ npm run bench

> globby@9.2.0 bench /Users/yhatt/Programs/globby
> npm update glob-stream fast-glob && matcha bench.js

+ fast-glob@3.0.2
added 6 packages from 5 contributors, removed 3 packages and updated 7 packages in 4.19s
(node:13488) [DEP0006] DeprecationWarning: child_process: options.customFds option is deprecated. Use options.stdio instead.

                      negative globs (some files inside dir)
             682 op/s » globby async (working directory)
             216 op/s » globby async (upstream/master)
             992 op/s » globby sync (working directory)
             285 op/s » globby sync (upstream/master)
             250 op/s » glob-stream
           1,013 op/s » fast-glob async
           1,182 op/s » fast-glob sync

                      negative globs (whole dir)
             904 op/s » globby async (working directory)
             252 op/s » globby async (upstream/master)
           1,061 op/s » globby sync (working directory)
             294 op/s » globby sync (upstream/master)
           5,097 op/s » glob-stream
           1,024 op/s » fast-glob async
           1,254 op/s » fast-glob sync

                      multiple positive globs
             498 op/s » globby async (working directory)
             120 op/s » globby async (upstream/master)
             489 op/s » globby sync (working directory)
             143 op/s » globby sync (upstream/master)
             160 op/s » glob-stream
             589 op/s » fast-glob async
             580 op/s » fast-glob sync


  Suites:  3
  Benches: 21
  Elapsed: 16,987.17 ms

I was received a report to my tool, about a wrong globbing by globby (marp-team/marp-cli#108). It's a bug from the upstream package so would be resolved by this.

fast-glob v3 does no longer convert slashes in glob pattern.
@yhatt

This comment has been minimized.

index.js Outdated
@@ -89,14 +90,16 @@ const getFilterSync = options => {
DEFAULT_FILTER;
};

const normalizeGlob = glob => glob.split(path.sep).join(path.posix.sep);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note:

Only forward-slashes in glob expression. Previously, we convert all slashes to the forward-slashes, which did not allow the use of escaping. See pattern syntax section in the README.md file. - https://github.com/mrmlnc/fast-glob/releases/tag/3.0.0

I don't think we should override that as the change was done for a good reason. Instead, we should update the tests and docs.

Copy link
Contributor Author

@yhatt yhatt Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. An only concern at here is the glob pattern generated by dir-glob. It includes unexpected backslashes because of using path.join in Windows. (kevva/dir-glob#17)

@sindresorhus
Copy link
Owner

New release of dir-glob is out: https://github.com/kevva/dir-glob/releases/tag/v3.0.0 Can you bump it here?

@yhatt
Copy link
Contributor Author

yhatt commented Jun 27, 2019

Hm, a known failure is pass only in Windows.

globby/test.js

Lines 188 to 195 in 8aadde8

test.failing('relative paths and ignores option', t => {
process.chdir(tmp);
t.deepEqual(globby.sync('../tmp', {
cwd: process.cwd(),
ignore: ['tmp']
}), []);
process.chdir(cwd);
});

@sindresorhus
Copy link
Owner

Remove the .failing modifier. It seems to be passing now.

@yhatt
Copy link
Contributor Author

yhatt commented Jun 27, 2019

It is still failing in Linux and macOS. I looked into the cause a little bit but I had no idea 🤔

@mrmlnc
Copy link
Contributor

mrmlnc commented Jun 27, 2019

Hi guys!

When I released fast-glob, I also tested its serviceability in the globby package with some patch:

Maybe this will help in some way. But it was monkey-patching 🐒

@mrmlnc
Copy link
Contributor

mrmlnc commented Jun 27, 2019

Hmmmm… looks strange for me.

Output for tasks:

globby/index.js

Line 131 in 8aadde8

const tasks = globTasks.reduce((tasks, task) => {

Windows (PowerShell):

{
  pattern: '../tmp',  // <-------------------------
  options: {
    ignore: [ 'tmp' ],
    expandDirectories: true,
    cwd: 'D:\\OpenSource\\globby2\\tmp'
  }
}

Ubuntu (WSL):

{ pattern: '../tmp/**',  // <-------------------------
  options:
   { ignore: [ 'tmp' ],
     expandDirectories: true,
     cwd: '/mnt/d/OpenSource/globby2/tmp' } }

Most likely this is related to the dir-glob package, but I have no free time right now to debug it :(

@yhatt
Copy link
Contributor Author

yhatt commented Jun 28, 2019

Because I just worked to fix generated patterns for Windows to be compatible with an updated micromatch, I did not exactly intent to resolve a known failing test or change existed behavior in globby by merging kevva/dir-glob#18.

It may take some time but I'm going to continue to debug...

@yhatt
Copy link
Contributor Author

yhatt commented Jun 29, 2019

Found out a reason!: kevva/dir-glob#18 (comment)

We can upgrade fast-glob by merging kevva/dir-glob#19 to dir-glob.

@sindresorhus
Copy link
Owner

@yhatt yhatt changed the title Upgrade fast-glob package to v3.0.2 Upgrade fast-glob package to v3.0.3 Jun 29, 2019
@yhatt
Copy link
Contributor Author

yhatt commented Jun 29, 2019

Thanks a lot 🙏 Bumped dir-glob and fast-glob.

@sindresorhus sindresorhus changed the title Upgrade fast-glob package to v3.0.3 Upgrade fast-glob package to v3 Jun 29, 2019
@sindresorhus sindresorhus merged commit 3706920 into sindresorhus:master Jun 29, 2019
@sindresorhus
Copy link
Owner

Released: https://github.com/sindresorhus/globby/releases/tag/v10.0.0

Thanks for the help 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants