perf: add React.lazy to the Routes #28172
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
HowardBraham
added
area-performance
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
Builds ready [7637683]
Page Load Metrics (1709 ± 68 ms)
Bundle size diffs
|
HowardBraham
force-pushed
the
react.lazy
branch
from
077b53d to
43d6800
Compare
Builds ready [3a1b168]
Page Load Metrics (1890 ± 136 ms)
Bundle size diffs
|
HowardBraham
requested review from
matthewwalsh0,
darkwing,
seaona and
NidhiKJha
HowardBraham
force-pushed
the
react.lazy
branch
from
3a1b168 to
ca01e2a
Compare
Builds ready [ca01e2a]
Page Load Metrics (1627 ± 38 ms)
Bundle size diffs
|
matthewwalsh0
previously approved these changes
Nov 4, 2024
| @@ -1 +1 @@ | |||
| export { Connections } from './connections'; | |||
There was a problem hiding this comment.
I hit this in my POC also, is there definitely no workaround for this as we tend to prefer non-default exports for auto-complete and extensibility?
There was a problem hiding this comment.
Okay I found this method and it seems to work: https://dev.to/iamandrewluca/react-lazy-without-default-export-4b65
So we would write it as
const Connections = React.lazy(() =>
import('../../components/multichain/pages/connections').then((module) => ({
default: module.Connections,
})),
);
But I'm not totally convinced we should do that. For one thing, it's kind of hard to follow. And second, people say it may limit tree-shaking.
ui/pages/routes/routes.component.js
Outdated
| @@ -122,6 +96,66 @@ import { | |||
| showOnboardingHeader, | |||
| } from './utils'; | |||
|
|
|||
| // Begin Lazy Routes | |||
| const OnboardingFlow = React.lazy(() => | |||
There was a problem hiding this comment.
Given the volume of changes here and limited readability, is it worth listing these in alphabetical order?
There was a problem hiding this comment.
Hah, I guess it looked a little chaotic to you, but actually I was following an order. The lazy route imports are in the same order as the <Switch> component.
ui/pages/routes/routes.component.js
Outdated
| const OnboardingFlow = React.lazy(() => | ||
| import('../onboarding-flow/onboarding-flow'), | ||
| ); | ||
| const Lock = React.lazy(() => import('../lock')); |
There was a problem hiding this comment.
Minor, but something I did in my POC was create a helper function for this so we had an easy injection point for logging and timing.
There was a problem hiding this comment.
This is a fascinating thought, and maybe the helper function could even handle the non-default-export thing.
7 tasks
HowardBraham
force-pushed
the
react.lazy
branch
2 times, most recently
from
882e28d to
52d363f
Compare
Builds ready [52d363f]
Page Load Metrics (1949 ± 56 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
force-pushed
the
react.lazy
branch
from
52d363f to
035d2f1
Compare
Builds ready [035d2f1]
Page Load Metrics (1995 ± 74 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
force-pushed
the
react.lazy
branch
from
035d2f1 to
580162d
Compare
Builds ready [580162d]
Page Load Metrics (1680 ± 87 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
force-pushed
the
react.lazy
branch
7 times, most recently
from
5ba5204 to
ff86b44
Compare
HowardBraham
force-pushed
the
react.lazy
branch
from
ff86b44 to
a5e2bd5
Compare
Builds ready [a5e2bd5]
Page Load Metrics (1778 ± 57 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
HowardBraham
force-pushed
the
react.lazy
branch
2 times, most recently
from
9c266ef to
8f4e4a4
Compare
… set, and then do it by Math.random probability
HowardBraham
force-pushed
the
react.lazy
branch
from
8f4e4a4 to
17fed26
Compare
Builds ready [17fed26]
Page Load Metrics (1474 ± 34 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [17fed26]
Page Load Metrics (1698 ± 56 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [1152c05]
Page Load Metrics (1729 ± 90 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
pedronfigueiredo
approved these changes
Nov 25, 2024
jonybur
suggested changes
Nov 25, 2024
| @@ -133,9 +133,7 @@ const ConfirmTransaction = () => { | |||
| }); | |||
|
|
|||
| useEffect(() => { | |||
| if (!totalUnapproved && !sendTo) { | |||
| history.replace(mostRecentOverviewPage); | |||
There was a problem hiding this comment.
We seem to be losing this redirect this is intended
jonybur
approved these changes
Nov 25, 2024
metamaskbot
added
the
release-12.9.0
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is the first PR to add
React.lazyto the codebase. The meat of the changes are inui/pages/routes/routes.component.js.Every other file changed is just a reaction to those changes. Much of this is caused by the fact that
React.lazyonly works on default exports (see the documentation here), so I had to change a few named exports to default exports.I don't think this PR has much of an impact on loading times, but it sets up further work that should have an impact.
There was one component (
ConfirmTransaction) that needed some extra attention to work withReact.lazy. I had to remove thehistory.replace(mostRecentOverviewPage);, which I'm pretty sure was an unnecessary statement, and remove the Unit Test related to it.UPDATE: I changed all of the Integration Tests to convert
getBy...toawait findBy...to wait for theReact.lazycomponent to load.Related issues
Progresses: MetaMask/MetaMask-planning#3302