[Do not land] Remove fast-glob #3074
Conversation
|
Everybody on a diet! 🙌 |
|
Let's try without flat map. @alloy do you know how to best verify this code doesn't break anything? We don't use this code path at fb 🤷♂️ |
|
It can probably be tested in https://github.com/artsy/reaction, let me see if I can quickly spin that up. |
There was a problem hiding this comment.
Copy-pasted this code into the bundle relay-compiler bin and didn’t see any changes. Here’s a gist that shows the original fast-glob results in the first revision and the new glob results in the second revision: https://gist.github.com/alloy/e256ddacd5c2c923df83e73d7de4d5fa/revisions.
| @@ -23,6 +23,7 @@ const WatchmanClient = require('../core/GraphQLWatchmanClient'); | |||
|
|
|||
| const crypto = require('crypto'); | |||
| const fs = require('fs'); | |||
| const glob = require('glob'); | |||
There was a problem hiding this comment.
Is there a reason to always require this rather than inline as before?
There was a problem hiding this comment.
I assume it doesn't matter but I don't know.
There was a problem hiding this comment.
I guess it was inlined because this code-path isn’t hit when you use watchman? Probably not a huge hit, if at all.
There was a problem hiding this comment.
Not sure if we do it for open-source builds, but internally at FB we automatically inline requires so the behaviour would be identical in that case.
There was a problem hiding this comment.
iirc inlining all requires was disabled, as it led to worse behaviour of relay-runtime in a SSR context. /cc @robrichard
|
Thank you so much for giving it a try <3 I'll land the internal version of this change after somebody approves it. |
|
@cpojer, Sorry for the mention. I'm the maintainer of the fast-glob package and I want to make this package better. Unfortunately, I don't have access to the facebook intranet. If you don't mind, tell me please, the decision to replace JFYI: The |
|
@mrmlnc I’m not at FB and don’t know for sure, but I believe it’s related to reducing the size of the overall dependency tree (especially in a react/native app), hence my comment about “diet”. |
|
@mrmlnc @alloy is correct. There is nothing wrong with your package, in fact I actually like it quite a bit. I have been reducing the size of Facebook's React Native node_modules folder internally at Facebook. We use a monorepo, so every product developer has to install dependencies for every single project: Relay, Metro, Jest, Babel, eslint, React Native, jscodeshift and dozens more. Previously the size of our node_modules folder was 930 MiB and we pulled in thousands of packages, many which were outdated or unnecessary. This lead to a long install time until people could get started, both on CI and locally. I have since been focusing on reducing the size of this folder through a number of (funky) techniques while simultaneously updating everything to the latest major versions. Our node_modules folder is now ~143 MiB and is much faster to install. One of the techniques was to eliminate dependencies that are big (500k+), used only in one or two places (for example only in Relay) and are already covered by functionality of packages that are shared across a wider range of tools (like "glob"). In this case the impact is very low as people who run into problems with glob in open source can switch to watchman, which for large projects is much more scalable. While part of the source code, we do not use this code path at Facebook either. To summarize, I'm removing large dependencies that we don't use (much) to make it faster for product developers to get started with React Native. If you are looking for things to improve I recommend reducing the size of the "fast-glob" packages and the dependencies it brings in. Why does it have to be 500k? Can it be 100k? Can it be 10? |
|
@cpojer great work, how are you tracking big packages inside node_modules? |
|
@cpojer, Thank you for the quick reply! In the Thank you again for the detailed answer. We will work on performance and «size» issues in the future 🎉 |
|
@mrmlnc Nice work! I'll definitely consider using |
See https://www.internalfb.com/intern/diff/D21428538