refactor: routes.component.js and creation of ToastMaster #27735
Conversation
HowardBraham
added
the
team-tiger
|
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. |
github-actions
bot
added
the
team-contributor-experience
HowardBraham
removed
the
team-contributor-experience
HowardBraham
force-pushed
the
refactor-routes
branch
from
45cbead
to
0eb601f
Compare
| export function setNewPrivacyPolicyToastShownDate(time: number) { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | ||
| } | ||
|
|
||
| export function setNewPrivacyPolicyToastClickedOrClosed() { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | ||
| } |
There was a problem hiding this comment.
I believe submitRequestToBackground would still need to be awaited/thenned even if it's no longer being called inside of a thunk callback.
As I understand it, the motivation for removing this function call from redux would be to turn the thunks it used to be in into synchronous actions, but this doesn't change the fact that submitRequestToBackground itself returns a promise.
| export function setNewPrivacyPolicyToastShownDate(time: number) { | |
| submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | |
| } | |
| export function setNewPrivacyPolicyToastClickedOrClosed() { | |
| submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | |
| } | |
| export async function setNewPrivacyPolicyToastShownDate(time: number) { | |
| await submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | |
| } | |
| export async function setNewPrivacyPolicyToastClickedOrClosed() { | |
| await submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | |
| } |
There was a problem hiding this comment.
@Gudahtt I don't think there's any reason to wait for this to complete, but what do you think?
There was a problem hiding this comment.
My understanding is also that any requests to the background rely on the JSON-RPC API over the port streams and are therefore asynchronous, meaning we should await since submitRequestToBackground returns a promise.
I recall a contribution guideline that also recommends awaiting even when returning as it results in better stack traces in the event of errors.
There was a problem hiding this comment.
I did a little research and experimentation on this, screenshots below. My conclusion is that our contributor guidelines may be incorrect, unless someone can show me a counterexample.
In this specific case, we actually do NOT want to wait for this. Waiting would slow down the main thread, and we don't need to wait for anything to happen before continuing.
Fake error in foreground, no async await
Fake error in foreground, with async await
Fake error in background, no async await
Fake error in background, with async await
There was a problem hiding this comment.
By newly introducing floating promises into the codebase, wouldn't we be triggering uncaught exceptions that will crash the process (like the ones shown in the screenshots), when before this wasn't a concern?
I assume that for these background requests the consistency requirements are so weak that we don't need to log their failures. By the same token, aren't they also unimportant enough that we shouldn't be crashing the app over their failure?
Regarding the screenshots, I'm finding it confusing to understand what the promise chains being tested are. In the experiments above, what would be the equivalents of a, b, c from https://mathiasbynens.be/notes/async-stack-traces?
There was a problem hiding this comment.
From that example, a is setNewPrivacyPolicyToastShownDate, b is submitRequestToBackground, and c is continuing on with function PrivacyPolicyToast(). But c doesn't have to wait for b to finish.
There was a problem hiding this comment.
I think we might be conflating the following in this discussion:
b.then(() => c());b.then(() => ...);
c();In the second example, execution for the b.then() clause does not synchronously block c.
With that in mind, how would you feel about at least removing the possibility of uncaught exceptions by adding error handling without relying on async/await syntax?
export function setNewPrivacyPolicyToastClickedOrClosed() {
submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed')
.catch((error) => ...);
}In general, floating promises are forbidden by our eslint config with the @typescript-eslint/no-floating-promises rule, which is also included in the recommended ruleset for @typescript-eslint. The only reason that error isn't showing up here is because extension is using eslint configs that are three years old.
Edit: The reason I'm mentioning the lint rule is because someone will be fixing this code down the line so that it conforms with no-floating-promises. If we want to avoid the code from being altered a comment should be left at the very least, or a catch clause added so this is not flagged as a violation. Adding error handling would be the safe thing to do as well.
There was a problem hiding this comment.
a is setNewPrivacyPolicyToastShownDate, b is submitRequestToBackground, and c is continuing on with function PrivacyPolicyToast()
Having a bit of trouble understanding this. Wouldn't PrivacyPolicyToast be a since it's the outermost scope that the other two are nested in? a being an async function that contains b, c seems like an unalterable part of the premise discussed in the article.
Maybe the second half of a that follows b could be formulated as c, but then we don't seem to be examining the stack trace for an error in c? Not to mention a isn't an async function since it's a react component.
This discussion probably belongs in a dedicated ticket in contributor-docs. I think having the test code available would be helpful as well.
| export function setNewPrivacyPolicyToastShownDate(time: number) { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | ||
| } | ||
|
|
||
| export function setNewPrivacyPolicyToastClickedOrClosed() { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | ||
| } | ||
|
|
||
| export function getNftDetectionEnablementToast(state: State): boolean { | ||
| return Boolean(state.appState?.showNftDetectionEnablementToast); | ||
| } | ||
|
|
||
| export function setShowNftDetectionEnablementToast( | ||
| value: boolean, | ||
| ): PayloadAction<string | ReactFragment | undefined> { | ||
| return { | ||
| type: SHOW_NFT_DETECTION_ENABLEMENT_TOAST, | ||
| payload: value, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The three setters here are actions not selectors. It looks like the first two are no longer returning callbacks for thunks but the last one is still a redux action. Should all three be in this file?
There was a problem hiding this comment.
Oh yes, I thought about this one. Do we prefer renaming this file to toast-master-selectors-and-actions.ts or creating a new toast-master-actions.ts?
There was a problem hiding this comment.
Both would work. I agree with the idea of removing these from the global actions.ts and co-locating them with other toast things.
Maybe a separate toast-master-actions.ts would be just a little bit better for readability.
There was a problem hiding this comment.
@Gudahtt I thought about it a little more, and actually, are plain submitRequestToBackground functions actions at all?
There was a problem hiding this comment.
Definitely would recommend a separate file for these, but no the submitRequestToBackground ones are not technically Redux actions as we're not dispatching them on a reducer, ultimately just utils.
Builds ready [0eb601f]
Page Load Metrics (1892 ± 86 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
gauthierpetetin
added
area-performance
HowardBraham
requested review from
matthewwalsh0,
davidmurdoch,
NidhiKJha and
Gudahtt
HowardBraham
force-pushed
the
refactor-routes
branch
from
5cc40b7
to
789eb79
Compare
Builds ready [789eb79]
Page Load Metrics (1815 ± 96 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
| export function setNewPrivacyPolicyToastShownDate(time: number) { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | ||
| } | ||
|
|
||
| export function setNewPrivacyPolicyToastClickedOrClosed() { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | ||
| } | ||
|
|
||
| export function getNftDetectionEnablementToast(state: State): boolean { | ||
| return Boolean(state.appState?.showNftDetectionEnablementToast); | ||
| } | ||
|
|
||
| export function setShowNftDetectionEnablementToast( | ||
| value: boolean, | ||
| ): PayloadAction<string | ReactFragment | undefined> { | ||
| return { | ||
| type: SHOW_NFT_DETECTION_ENABLEMENT_TOAST, | ||
| payload: value, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Definitely would recommend a separate file for these, but no the submitRequestToBackground ones are not technically Redux actions as we're not dispatching them on a reducer, ultimately just utils.
| export function setNewPrivacyPolicyToastShownDate(time: number) { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastShownDate', [time]); | ||
| } | ||
|
|
||
| export function setNewPrivacyPolicyToastClickedOrClosed() { | ||
| submitRequestToBackground('setNewPrivacyPolicyToastClickedOrClosed'); | ||
| } |
There was a problem hiding this comment.
My understanding is also that any requests to the background rely on the JSON-RPC API over the port streams and are therefore asynchronous, meaning we should await since submitRequestToBackground returns a promise.
I recall a contribution guideline that also recommends awaiting even when returning as it results in better stack traces in the event of errors.
HowardBraham
force-pushed
the
refactor-routes
branch
2 times, most recently
from
ab9ab05
to
553bbe2
Compare
Builds ready [553bbe2]
Page Load Metrics (1858 ± 70 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
HowardBraham
force-pushed
the
refactor-routes
branch
3 times, most recently
from
965772e
to
7a54b36
Compare
Builds ready [7a54b36]
Page Load Metrics (2010 ± 314 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
HowardBraham
force-pushed
the
refactor-routes
branch
2 times, most recently
from
271da67
to
2aad6bf
Compare
Builds ready [2aad6bf]
Page Load Metrics (1779 ± 41 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
HowardBraham
force-pushed
the
refactor-routes
branch
from
65bbd8c
to
bfd7b86
Compare
Builds ready [bfd7b86]
Page Load Metrics (1828 ± 76 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Description
This PR is part of the larger effort to add
React.lazyto routes.I would be very surprised if I did not break functionality somewhere, as this was a large and tough refactor. It needs some good QA, and Harika has been doing some testing that has passed.
Summary of what I did:
For later PRs:
Related issues
Progresses: MetaMask/MetaMask-planning#2898
Manual testing steps
Test all the Routes and all the Toasts
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist