-
Notifications
You must be signed in to change notification settings - Fork 902
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
perf: replace glob with fast-glob #2280
Conversation
22835e1
to
17610cd
Compare
There was a previous PR for this which went stale which mentioned that same issue - there might be a solution in there already? #2016 |
hey @abejfehr! thanks for contribution, could you please rebase on top of |
17610cd
to
c72f1d6
Compare
Thanks! I've rebased. Looking at the prior art helped me resolve the FS mocking issue and it passes tests now locally, so I think it's ready for a review |
@abejfehr thanks! looks like Windows E2E is failing now, same as in my PR 🙈 Could you please look at this? I'll also try to search for solution for this. |
I ran the tests on a Windows machine and noticed that it wasn't building properly, and it looks like that's because I replaced glob with fast-glob in I opted for adding Now on Windows for me there's ~3 failing tests but they seem to be due to very specific paths and the ones in |
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.
Thank you @abejfehr! 🙏 CI Green ✅
Summary:
Closes #2271
I was investigating how
pod install
was so slow in React Native, and it seems like a lot of time is spent inIO#gets
:Upon further investigation, this is actually Ruby waiting for the output of
node .../node_modules/@react-native-community/cli/build/bin.js config
.When investigating what's slow about
node .../node_modules/@react-native-community/cli/build/bin.js config
, I noticed that a lot of time was spent in globs (about ~17 seconds on an M1 Mac):In my testing, I've observed that
glob
can be replaced withfast-glob
to make performance considerably better with the same output, so I'd like to propose thatglob
be replaced withfast-glob
in this project.Test Plan:
The unit tests are great, but one thing I'm encountering is that the FS mock doesn't seem to play well with something that fast-glob is doing internally.
Even though it works in practice, it doesn't seem to work in tests 😢 it's like
fs.__setMockFilesystem
isn't taking effect or something (it is once, but never subsequently. It's like fast-glob doesn't re-scan the disk? idk)Any help here would be greatly appreciated.
Checklist
Documentation is up to date to reflect these changes.