[RFC DRAFT WIP] globby@10 update (with fast-glob implementation) #6748

brodybits · 2019-10-29T20:34:20Z

use globby@10 (globby@10.0.1) which has fast-glob implementation
some minor test snapshot updates in tests_integration since fast-glob does not always keep the same file order as node-glob

I am raising this PR for the sake of testing and discussion related to PR #6639 (globby@7 update).

This seems to work from git on macOS and Linux, mostly working from git on Windows.

P.S. If we do this update, there should be no need to revert PR #6333 as discussed in #6719 / #6721

TODO items:

resolve performance issue with finding plugins
consider updating CHANGELOG.unreleased.md with information about switch to fast-glob implementation
consider updating doc that fast-glob is only used in the new release ref: Link to fast-glob instead of glob in docs #6333 /this PR need revert #6719 / Revert "Link to fast-glob instead of glob in docs (#6333)" #6721

/cc @fisker @lydell @evilebottnawi

~~I’ve added tests to confirm my change works.~~
~~(If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)~~
(If the change is user-facing) I’ve added my changes to the CHANGELOG.unreleased.md file following the template.
I’ve read the contributing guidelines.

✨Try the playground for this PR✨

fisker · 2019-10-30T02:07:59Z

issue with absolute path integration test on Windows

add

patterns = patterns.map(path => path.replace(/\\/g, '/'))

here ,

prettier/src/cli/util.js

Line 403 in 6d4d6a7

function eachFilename(context, patterns, callback) {

or follow this guide https://github.com/mrmlnc/fast-glob#how-to-write-patterns-on-windows

fisker · 2019-10-30T02:25:02Z

Another thing need attention https://dev.azure.com/prettier/prettier/_build/results?buildId=2385

this is taking tooooo long, the reason why it's too slow is because of

prettier/src/common/load-plugins.js

Lines 97 to 104 in 3805034

    
           const pluginPackageJsonPaths = globby.sync( 
        
             [ 
        
               "prettier-plugin-*/package.json", 
        
               "@*/prettier-plugin-*/package.json", 
        
               "@prettier/plugin-*/package.json" 
        
             ], 
        
             { cwd: nodeModulesDir } 
        
           );

but I'm not sure about the logic, do we find plugins in node_modules in any deepth or just root

fisker · 2019-10-30T02:31:02Z

about regeneratorRuntime, I think we can update babel settings, like this

https://github.com/fisker/promisify-file-reader/blob/master/babel.config.js#L9-L16

just exclude transform-regenerator and add babel-plugin-transform-async-to-promises

brodybits · 2019-10-30T03:54:29Z

add

patterns = patterns.map(path => path.replace(/\\/g, '/'))

Thanks @fisker, seems to do the trick (with a couple Prettier lint fixes applied). I had to apply a few more updates to get some consistent results on Windows and Linux:

revert the removal of nodir: true option
add sorting of the file paths from globby (ordering did not seem to be consistent on Windows & Linux)
revert test snapshot updates that are no longer needed

We will now see if the build results on Windows, Linux, and macOS are really consistent or not.

About the other points: unfortunately I am not so familiar with these. @fisker I wouldn't mind if you want to take this update over.

I am now starting to wonder how much we really want to switch to the fast-glob implementation, given that it is still known to be slower, seems to suffer from performance issues in certain cases, and needs special support for regeneratorRuntime. I wonder if we should consider switching to async functions (or maybe just use promises) to keep up with the newer library updates?

fisker · 2019-10-30T05:37:45Z

@brodybits brodybits#9

fisker · 2019-10-30T09:45:17Z

revert the removal of nodir: true option

there is no nodir option in v10 , same option is onlyFiles, and this default true https://github.com/mrmlnc/fast-glob#compatible-with-node-glob

fisker · 2019-10-30T09:48:47Z

about this

prettier/src/common/load-plugins.js

Lines 97 to 104 in 3805034

    
           const pluginPackageJsonPaths = globby.sync( 
        
             [ 
        
               "prettier-plugin-*/package.json", 
        
               "@*/prettier-plugin-*/package.json", 
        
               "@prettier/plugin-*/package.json" 
        
             ], 
        
             { cwd: nodeModulesDir } 
        
           );

I thought it's only 1, 2 or 3 depth, trying depth:1, and there is already tests in tests_integration/plugins/automatic

brodybits · 2019-10-30T16:36:58Z

@brodybits brodybits#9

Thanks @fisker for the updates, I pushed these to my branch almost an hour ago.

I am finding it kinda strange that I get a green build on this PR, while I get a few tests errors in both my local workarea (on Linux) and brodybits#9. I will probably need some time to investigate these, any ideas would be much helpful.

Here are the next steps I am planning to take:

pull updates from master
rebase to cleanup some of the history

fisker · 2019-10-30T16:52:54Z

@brodybits merge master into this branch will start the ci test runing , I have some issue with my laptop cant work on this now. you can continue work on this

fisker · 2019-10-30T16:59:37Z

src/common/load-plugins.js

-  return pluginPackageJsonPaths.map(path.dirname);
+  return pluginPackageJsonPaths
+    .map(packageJsonFile => path.dirname(packageJsonFile.slice(2)))
+    .sort((a, b) => a.toLowerCase().localeCompare(b.toLowerCase()));
 }


something is not right here, you can work on this, restore this will pass all test, but very slow

Thanks @fisker, I will take a look at this. I may need to ask for some more hints online (I will deal with the code changes if needed).

Sorry to hear about the troubles with your laptop. I am also having some troubles, using a virtual server to help with development on this.

Its late in China, going to sleep now, will back online in 9 hours, I was trying to make this fast. maybe the slice is not right just need check the glob result. lets see how fast it will be

fisker · 2019-10-30T17:01:30Z

and you can also try brodybits#9 (comment) , should be good and no need update snap

brodybits · 2019-10-30T17:13:49Z

@fisker I have a couple more questions about the updates from brodybits#9 and using fast-glob in general:

What is the effect on the build for the online playground (browser build)? Any pointers to help me understand this part?
Is there anything we can revert or remove once Node.js 6 support is completely gone?

fisker · 2019-10-30T17:19:21Z

What is the effect on the build for the online playground (browser build)? Any pointers to help me understand this part?

the build scripts changes effects two things

new package use util.promisify that node4 dont has, so I add pify to replace it
there is async function, babel transform into generater by default, I use a plugin to transform them into promise

Is there anything we can revert or remove once Node.js 6 support is completely gone?

just pify thing

to work more like a pipeline

using babel-plugin-transform-async-to-promises@0.8.15 Co-authored-by: fisker Cheung <lionkay@gmail.com> Co-authored-by: Christopher J. Brody <chris@brody.consulting>

brodybits · 2019-10-30T21:28:25Z

I made some updates as discussed in #6748 (comment) and did a clean rebase. The build is green now.

Thanks @fisker for answering my questions.

At this point I am not so enthusiastic about the new globby update due to the known performance disadvantages. I wish they offered an option to go back to node-glob which would be a little more deterministic (no need to use sort to get deterministic output) and not suffer from the performance drawbacks.

@fisker if you agree with this, then maybe one of us could raise an issue on globby?

I am now thinking we should consider using globby@7 for now. I would be happy to raise a new PR to do this, if people would be interested.

using pify@4.0.1 as needed to support build for Node.js version 6 This change may be reverted upon the end of Node.js 6 support. Co-authored-by: fisker Cheung <lionkay@gmail.com> Co-authored-by: Christopher J. Brody <chris@brody.consulting>

sort the file paths from globby and update some snapshots in tests_integration `\\` pattern workaround for fast-glob on Windows 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>

fisker · 2019-10-30T23:38:07Z

Dont give up so quickly I 'll try something else today

fisker · 2019-10-31T06:22:34Z

@brodybits can you pull https://github.com/fisker/prettier/tree/globby-10-update-with-fast-glob , I prefer keep the commit history, we always do squash and merge, no need to keep this clean

fisker · 2019-10-31T06:34:50Z

We need review logic from #4192

The reason I change searching package.json to searching directory is fast-glob is really show without deep in a big dir like node_modules.

We'll discuss this later

fisker · 2019-10-31T06:54:13Z

related issues and PRs

support dot pattern for CLI #6085 should be fixed
Update cli.md to explain how to fix all files #6285 need changing prettier --write "**" to prettier --write .
see also Update cli.md to explain how to fix all files #6285
maybe also this big ignore paths slow down cli #4989 ?

lydell · 2019-10-31T07:16:32Z

Maybe we should leave globby at the current version until we improve the CLI in 2.0 or 3.0 or something.

fisker · 2019-10-31T07:24:58Z

@lydell why? update globby can benefits file globbing for users, current problem only effect plugin searching logic, we can stil use node-glob for this part, or even no glob at all, just need a better solution. let's discuss when my newly code get pulled here

lydell · 2019-10-31T16:49:19Z

Because it sounds like it’s very difficult to upgrade and that the newer globby versions are slower. So it only sounds like we risk messing something up for users for no benefit (unless I’m missing something?)

brodybits · 2019-10-31T17:25:45Z

Thanks @fisker for all of your work on this.

At this point I am with @lydell to hold off on this update for now. I am really not so thrilled by the number of workarounds we have to do to deal with fast-glob. I think we should just require and use packages from npm, more complex I/O operations should be moved into reusable npm packages.

I would like to take a look through the updates, which seem to be in fisker#120, would like to work on some other priorities for now.

I would personally favor globby@7 update for now, in order to fall less far behind.

@fisker please feel free to open another new PR if you want to continue with this proposal. I would be happy to help review it if you like.

@lydell I have an off-topic comment with a question coming up.

brodybits · 2019-10-31T17:32:03Z

Off-topic for @lydell:

I really think we need to start a new major release asap to do the following:

completely drop support for old Node.js versions
consider "breaking" dependency updates
consider any other breaking changes that should be done with very limited controversy

Relying on old dependency versions makes the update increasingly more difficult over time. Supporting old Node.js versions makes this even worse. This seems to be a major diversion of time and attention that could be used for improvements and fixes.

It is not clear to me if and where this is being discussed. I would be happy to raise a new issue, if needed. Pointers would be much appreciated.

/cc @prettier/core

lydell · 2019-10-31T17:53:06Z

@brodybits I completely agree with you. That’s exactly what we should be doing after 1.19. Ideally we’d release 2.0 early January 2020 with support for Node.js 10+.

brodybits · 2019-10-31T17:56:33Z

Ideally we’d release 2.0 early January 2020 with support for Node.js 10+.

Is this discussed in one place somewhere?

Is there anything special that an outside contributor such as @fisker or I should do to propose breaking or potentially breaking changes for 2.0?

lydell · 2019-10-31T18:01:13Z

Is this discussed in one place somewhere?

Unfortunately not. I’d suggest opening an issue like #3503 with suggested breaking changes to start a discussion.

fisker · 2019-10-31T23:52:07Z

@lydell @brodybits

like it’s very difficult to upgrade

its not , I already did all the research and know how to make it work

that the newer globby versions are slower

indeed it is slower in certain case like our plugin searching , but most cases fast-glob is faster

I will post a snippet to prove this, when i got on laptop

for plugin searching , I did make a benchmark to test but forgot to push to github. as you can see on fisker#120 ci is not slow anymore, but maybe best way is not touch this part, I am planning to use node-glob for this part.

fisker · 2019-11-01T02:23:43Z

for speed test, use your prettier codebase to test scripts bellow

node -e "console.time();require('glob').sync('**/*.js');console.timeEnd();"
# default: 4233.523ms

node -e "console.time();require('fast-glob').sync('**/*.js');console.timeEnd();"
# default: 852.121ms

test on this glob I always use

https://github.com/fisker/imagemin-upng/blob/2dffd4b52e020151d55a39a1ddb1c42a305cdd15/package.json#L43

node -e "console.time();require('glob').sync('**/*.{css,html,js,json,less,md,scss,ts,vue,yaml,yml}');console.timeEnd();"
# default: 10177.959ms

node -e "console.time();require('fast-glob').sync('**/*.{css,html,js,json,less,md,scss,ts,vue,yaml,yml}');console.timeEnd();"
# default: 845.426ms

feel free to test on any real world glob patterns.

for plugin search, fast-glob is slower, but this function can refactor

node -e "console.time();require('glob').sync('{*,@*/*}/package.json', {cwd: 'node_modules'});console.timeEnd();"
# default: 96.961ms

node -e "console.time();require('fast-glob').sync('{*,@*/*}/package.json', {cwd: 'node_modules'});console.timeEnd();"
# default: 261.463ms

results are on my laptop, windows 10 and node 10

lydell · 2019-11-01T07:00:06Z

Cool, thanks. You’re testing glob and fast-glob directly and not globby, though, but hopefully using globby directly gives the same results.

fisker · 2019-11-01T08:06:03Z

yarn add globby7@npm:globby@7 globby10@npm:globby@10

then

E:\project\@forks\prettier>node -e "console.time();require('globby7').sync('**/*.js');console.timeEnd();"
default: 5568.626ms

E:\project\@forks\prettier>node -e "console.time();require('globby10').sync('**/*.js');console.timeEnd();"
default: 923.822ms

E:\project\@forks\prettier>node -e "console.time();require('globby7').sync('**/*.{css,html,js,json,less,md,scss,ts,vue,yaml,yml}');console.timeEnd();"
default: 10517.388ms

E:\project\@forks\prettier>node -e "console.time();require('globby10').sync('**/*.{css,html,js,json,less,md,scss,ts,vue,yaml,yml}');console.timeEnd();"
default: 997.479ms

E:\project\@forks\prettier>node -e "console.time();require('globby7').sync('{*,@*/*}/package.json', {cwd: 'node_modules'});console.timeEnd();"
default: 141.681ms

E:\project\@forks\prettier>node -e "console.time();require('globby10').sync('{*,@*/*}/package.json', {cwd: 'node_modules'});console.timeEnd();"
default: 311.261ms

@lydell

lydell · 2019-11-01T16:33:53Z

Nice! Seems like newer globby is usually a lot faster then.

mrmlnc · 2019-11-07T11:39:18Z

JFYI

I'm a fast-glob maintainer.

The fast-glob package in most cases, faster than competitors. But he have a some performance problems for complex patterns (mrmlnc/fast-glob#156) like {*,@*/*} or mrmlnc/fast-glob#156 (comment). And, yeap, we will work on it.

brodybits · 2019-11-07T23:55:38Z

Closing in favor of #6776 by @fisker.

@mrmlnc let's move your comment #6748 (comment) to PR #6776.

@fisker I find all of your old changes to be really hard to follow, would favor it if you would try to clean them up.

fisker · 2019-11-08T00:01:29Z

@brodybits I won't clean it, I can check what I've tryied

brodybits changed the title ~~[RFC ...] globby@10 update (with fast-glob implementation)~~ [RFC DRAFT WIP] globby@10 update (with fast-glob implementation) Oct 29, 2019

brodybits mentioned this pull request Oct 29, 2019

Update globby to v7.1.1 #6639

Closed

4 tasks

fisker reviewed Oct 30, 2019

View reviewed changes

Christopher J. Brody and others added 2 commits October 30, 2019 15:32

prep cleanup: update findPluginsInNodeModules()

fb38398

to work more like a pipeline

build prep: update build script to support async

dad750f

using babel-plugin-transform-async-to-promises@0.8.15 Co-authored-by: fisker Cheung <lionkay@gmail.com> Co-authored-by: Christopher J. Brody <chris@brody.consulting>

brodybits force-pushed the globby-10-update-with-fast-glob branch from 01c94f1 to f945854 Compare October 30, 2019 20:58

Christopher J. Brody and others added 2 commits October 30, 2019 17:47

build prep: replace util.promisify with pify

1a2cee5

using pify@4.0.1 as needed to support build for Node.js version 6 This change may be reverted upon the end of Node.js 6 support. Co-authored-by: fisker Cheung <lionkay@gmail.com> Co-authored-by: Christopher J. Brody <chris@brody.consulting>

brodybits force-pushed the globby-10-update-with-fast-glob branch from f945854 to 3d349f4 Compare October 30, 2019 21:55

brodybits mentioned this pull request Oct 31, 2019

Prettier 2.0 (old) #3503

Closed

lydell mentioned this pull request Nov 5, 2019

Bump globby from 6.1.0 to 10.0.1 #6820

Closed

brodybits closed this Nov 7, 2019

brodybits mentioned this pull request Nov 7, 2019

Update globby to v11 #6776

Merged

4 tasks

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Feb 6, 2020

lock bot locked as resolved and limited conversation to collaborators Feb 6, 2020

[RFC DRAFT WIP] globby@10 update (with fast-glob implementation) #6748

[RFC DRAFT WIP] globby@10 update (with fast-glob implementation) #6748

Conversation

brodybits commented Oct 29, 2019 • edited

fisker commented Oct 30, 2019 • edited

fisker commented Oct 30, 2019

fisker commented Oct 30, 2019

brodybits commented Oct 30, 2019

fisker commented Oct 30, 2019

fisker commented Oct 30, 2019

fisker commented Oct 30, 2019 • edited

brodybits commented Oct 30, 2019

fisker commented Oct 30, 2019

fisker Oct 30, 2019

Choose a reason for hiding this comment

brodybits Oct 30, 2019

Choose a reason for hiding this comment

fisker Oct 30, 2019

Choose a reason for hiding this comment

fisker commented Oct 30, 2019

brodybits commented Oct 30, 2019

fisker commented Oct 30, 2019 • edited

brodybits commented Oct 30, 2019 • edited

fisker commented Oct 30, 2019

fisker commented Oct 31, 2019

fisker commented Oct 31, 2019

fisker commented Oct 31, 2019 • edited

lydell commented Oct 31, 2019

fisker commented Oct 31, 2019 • edited

lydell commented Oct 31, 2019

brodybits commented Oct 31, 2019

brodybits commented Oct 31, 2019

lydell commented Oct 31, 2019

brodybits commented Oct 31, 2019

lydell commented Oct 31, 2019

fisker commented Oct 31, 2019

fisker commented Nov 1, 2019 • edited

lydell commented Nov 1, 2019

fisker commented Nov 1, 2019

lydell commented Nov 1, 2019

mrmlnc commented Nov 7, 2019 • edited

brodybits commented Nov 7, 2019

fisker commented Nov 8, 2019

brodybits commented Oct 29, 2019 •

edited

fisker commented Oct 30, 2019 •

edited

fisker commented Oct 30, 2019 •

edited

fisker commented Oct 30, 2019 •

edited

brodybits commented Oct 30, 2019 •

edited

fisker commented Oct 31, 2019 •

edited

fisker commented Oct 31, 2019 •

edited

fisker commented Nov 1, 2019 •

edited

mrmlnc commented Nov 7, 2019 •

edited