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
[RFC DRAFT WIP] globby@10 update (with fast-glob implementation) #6748
[RFC DRAFT WIP] globby@10 update (with fast-glob implementation) #6748
Conversation
add patterns = patterns.map(path => path.replace(/\\/g, '/')) here , Line 403 in 6d4d6a7
or follow this guide https://github.com/mrmlnc/fast-glob#how-to-write-patterns-on-windows |
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
but I'm not sure about the logic, do we find plugins in |
about https://github.com/fisker/promisify-file-reader/blob/master/babel.config.js#L9-L16 just exclude |
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:
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? |
there is no |
about this prettier/src/common/load-plugins.js Lines 97 to 104 in 3805034
I thought it's only 1, 2 or 3 depth, trying depth:1, and there is already tests in |
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:
|
@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 |
return pluginPackageJsonPaths.map(path.dirname); | ||
return pluginPackageJsonPaths | ||
.map(packageJsonFile => path.dirname(packageJsonFile.slice(2))) | ||
.sort((a, b) => a.toLowerCase().localeCompare(b.toLowerCase())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something is not right here, you can work on this, restore this will pass all test, but very slow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
and you can also try brodybits#9 (comment) , should be good and no need update snap |
@fisker I have a couple more questions about the updates from brodybits#9 and using fast-glob in general:
|
the build scripts changes effects two things
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>
01c94f1
to
f945854
Compare
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>
f945854
to
3d349f4
Compare
Dont give up so quickly I 'll try something else today |
@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 |
We need review logic from #4192 The reason I change searching We'll discuss this later |
related issues and PRs
|
Maybe we should leave globby at the current version until we improve the CLI in 2.0 or 3.0 or something. |
@lydell why? update |
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?) |
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. |
Off-topic for @lydell: I really think we need to start a new major release asap to do the following:
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 |
@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+. |
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? |
Unfortunately not. I’d suggest opening an issue like #3503 with suggested breaking changes to start a discussion. |
its not , I already did all the research and know how to make it work
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. |
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 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, 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 |
Cool, thanks. You’re testing |
yarn add globby7@npm:globby@7 globby10@npm:globby@10 then
|
Nice! Seems like newer globby is usually a lot faster then. |
JFYI
The |
@brodybits I won't clean it, I can check what I've tryied |
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:
CHANGELOG.unreleased.md
with information about switch to fast-glob implementationfast-glob
instead ofglob
in docs #6333 /this PR need revert #6719 / Revert "Link tofast-glob
instead ofglob
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 thedocs/
directory)CHANGELOG.unreleased.md
file following the template.✨Try the playground for this PR✨