-
-
Notifications
You must be signed in to change notification settings - Fork 273
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
Update react scripts #5908
Update react scripts #5908
Conversation
783c809
to
2364ced
Compare
2364ced
to
73d6ed3
Compare
Shouldn't the CRACO configuration file have been included in this PR? |
73d6ed3
to
67c144d
Compare
Yep. It looks like I messed up when I was cherry-picking the commit; I didn't run |
Let's also include the fix 9ed1d0c for the testimonial image in this PR? |
frontend/package.json
Outdated
"start": "npm run preparation && npm run copy-static && npm run patch-id && npm run patch-rapid && react-scripts start", | ||
"build": "npm run preparation && npm run update-static && npm run patch-id && npm run patch-rapid && react-scripts build", | ||
"start": "npm run preparation && npm run copy-static && npm run patch-id && npm run patch-rapid && craco start", | ||
"build": "npm run preparation && npm run update-static && npm run patch-id && npm run patch-rapid && craco build --stats", |
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.
To achieve quicker and leaner production bundles, wouldn't it be desirable to omit the --stats
flag?
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.
To achieve quicker [...] production bundles, wouldn't it be desirable to omit the --stats flag?
Surprisingly, no.
Here are some test build times (using time
, and specifically the total time elapsed). Starting with run 22, the runs were with all other applications closed and the computer locked.
Runtime statistics for no --stats
and --stats
Run | No stats (s) | Stats (s) |
---|---|---|
1 | 129.38 | 187.45 |
2 | 110.25 | 76.32 |
3 | 107.88 | 76.46 |
4 | 96.8 | 81.37 |
5 | 97.73 | 79.5 |
6 | 131.19 | 79.67 |
7 | 111.21 | 79.06 |
8 | 95.57 | 80.03 |
9 | 92.99 | 78.9 |
10 | 89.16 | 80.49 |
11 | 93.77 | 81.54 |
12 | 96.78 | 80.46 |
13 | 112.91 | 80.05 |
14 | 95.33 | 80.15 |
15 | 86.28 | 81.81 |
16 | 91.38 | 81.74 |
17 | 88.22 | 81.17 |
18 | 96.19 | 80.85 |
19 | 96.58 | 78.06 |
20 | 93.15 | 80.99 |
21 | 91.93 | 79.3 |
22 | 84.5 | 75.25 |
23 | 87.12 | 79.56 |
24 | 87.97 | 78.61 |
25 | 84.9 | 79.1 |
26 | 84.71 | 79.02 |
27 | 85.36 | 77.64 |
28 | 82.91 | 57.84 |
To achieve [...] leaner production bundles, wouldn't it be desirable to omit the --stats flag?
build
is 339M for --stats
with 107M from the stats file, and 227M without the --stats
. With that said, the numbers really need to be compared over multiple runs. And isn't what would reach the users anyway.
For reference, build
is 228M with current develop
.
EDIT: Change summary for runtime details to match table headers.
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.
But isn't it evident from the table that excluding the "--stats" flag generally led to shorter build times?
Could you kindly clarify why you mentioned that excluding the "--stats" flag might not be desirable? I value your insights and would appreciate further clarification.
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.
But isn't it evident from the table that excluding the "--stats" flag generally led to shorter build times?
That wasn't the case. I probably should have flipped the description (--stats
and no --stats
-> no --stats
and --stats
) to match the table headers.
The average for no --stats
was 96.5s and for --stats
it was 82.5s. The median for no --stats
was 93.46s and the median for --stats
was 79.6s.
It does produce a slightly larger build
directory, most of which is from the bundle-stats.json
file (which doesn't need to be served). I should probably do another 10 runs of --stats
and no --stats
to see how the build
directory size changes and whether or not it is consistent between runs.
EDIT: To clarify, I'm not necessarily advocating keeping or dropping --stats
; I understand that different hardware and software combinations can produce different results. I would hope that the results of for i in {0..20}; do /usr/bin/time -a -o ${timings_type}.txt yarn build; done
is reproducible across systems, in which case dropping --stats
would increase build times while also increasing build
directory size.
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.
I was primarily only trying to understand whether including the --stats
flag would make a significant difference. Considering the insights you've provided and the analysis conducted, it seems reasonable to proceed with merging the pull request as it is. Does anything need to be addressed to prevent the stats file from being served?
Thank you for your valuable contributions to this conversation. Are there any additional considerations before merging?
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.
Does anything need to be addressed to prevent the stats file from being served?
Probably. I'll take a quick look into it. Realistically, we could just do an rm
in the CI prior to deployment. But that probably isn't the best option. Something that might be better is a check, e.g. if [ "${NODE_ENV}" == "PRODUCTION" ] ; then craco build; else craco build --stats; fi
. That works on mac/linux, but might not work on Windows.
With that said, I think there is a ticket open for switching to Terraform and there is a repo for that.
c7e5eb6
to
f5b9cdf
Compare
73e0f86
to
04aa8a6
Compare
@tsmock, would it be okay to merge the pull request and address the --stats flag in a subsequent update? |
Works for me. I'm currently checking to make certain that tests didn't get borked on my last rebase. |
04aa8a6
to
1c57d20
Compare
I've removed the |
We might want to hold off on merging this -- I just fixed a bug in @placemarkio/geo-viewport w.r.t. webpack 5. The fix has been merged, but is not yet released. If you run |
2c6a174
to
3df3bfc
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
|
The tests pass successfully, but the CircleCI process |
Looking at jestjs/jest#9324, it seems like there might be a test that is doing something wrong that might be causing the failure. I'm currently in the process of bisecting the test files, and I'm probably going to have to do that multiple times. EDIT: It looks like the |
3df3bfc
to
fdf375b
Compare
Signed-off-by: Taylor Smock <tsmock@meta.com>
* timers (such as advanceTimersByTime) * clicks Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
…zero exit code See jestjs/jest#10728 for additional details (in jest 27) Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
Signed-off-by: Taylor Smock <tsmock@meta.com>
5268dde
to
ce942ab
Compare
Quality Gate passedIssues Measures |
Hi @tsmock what's the view of yours on transitioning to vite? Will it be a lot of work or something i don't know ? Just a query |
Doing it manually will be a lot of work. I looked into it a bit awhile ago (sometime last year). I don't remember why I dropped it (it might have been not wanting to have to do another massive PR while I had one in progress). In my investigations, I found that someone wrote a script to eject to vite (https://github.com/bhbs/viject), so that will take care of the bulk of the work. Things to note:
|
Hmmm the viject tools seems interesting, i will have a look into it soon. But yeah so for now are we be moving ahead with this PR ? and then onwards try to switch it to vite, as i think we will be using alot of vite in other product of HOT aswell so maybe something to be considered ? Vite has a plugin for routing i guess which recently changed it's name to Vike. At the moment for moving forward on the TM for vite or upgrading the react-scripts/craco what's your take , let me know further on this if its feasible or not? |
This was largely copied from #5721 so that we could build/run on Node 18+ without TBH, I was using What I would do is the following:
Either way, I would strongly advocate moving away from With that said, it sounds like other HOT projects are using vite. So that is probably the winner. |
Hmmm sounds good will let ramya know on this aswell. |
Still relevant @varun2948 ? |
No, this PR is no longer relevant; the changes from this PR were in #5721. |
This updates react-scripts from 4.0.3. to 5.0.1. Due to some issues with react-scripts, CRACO was added to work around an issue with svg images. CRACO did cause the main bundle size to increase significantly to 6 MB, and the main bundle size can reduced via lazy loading in the router (see #5914). There is more in-depth analysis in that PR on decreasing the initial size of all bundles needed for the initial render.