Conversation
Builds ready [dab8bdb]
Page Load Metrics (582 ± 35 ms)
|
Builds ready [e71bcb0]
Page Load Metrics (601 ± 33 ms)
|
Builds ready [9e5d8d1]
Page Load Metrics (569 ± 60 ms)
|
Gudahtt
left a comment
There was a problem hiding this comment.
Looks good aside from the redundant state update part! Though I didn't test this locally.
Builds ready [97be263]
Page Load Metrics (534 ± 47 ms)
|
|
|
||
| state = { | ||
| mounted: false, | ||
| canShowBlockageNotification: true, |
There was a problem hiding this comment.
This might be pretty annoying for users, as it'll be restored every time they navigate back to the home page. e.g. "Home => Settings => Home" will force them to dismiss it again.
But nonetheless, I like this solution for now. It's simple and pragmatic. We can improve the dismissal logic later if we feel the need. Given that there's not a lot users can do in this state, it might not be super important to improve this.
There was a problem hiding this comment.
My exact thoughts here as well, I think when we approach improving network connectivity UX as a whole, this should definitely be a point for refactoring
Builds ready [538fea7]
Page Load Metrics (939 ± 79 ms)
|
Gudahtt
left a comment
There was a problem hiding this comment.
I've discovered a problem: the net_version request never goes to Infura. We intercept it and return the constant we have for the network version.
The two options I can see are checking the block tracker requests for fetch failures, or making a separate fetch request each time the network changes. The former is much nicer idea because it saves network traffic but unfortunately the fetch error details aren't returned by the block tracker at the moment - that would require an update to that library. Which we should absolutely do, but we don't have time for that now. So, direct fetch it is I suppose.
I'd suggest making it a non-blocking fetch as well, so that we don't slow down the network switch process.
I created a mock Infura server to test this with, which is available here: https://github.com/MetaMask/metamask-extension/compare/mock-infura . It also includes a patch to update eth-json-rpc-infura to send request to localhost instead of to Infura. That patch might not be necessary if you're calling fetch directly but it was important for testing anything that relies upon our provider.
Builds ready [6bcde24]
Page Load Metrics (1019 ± 59 ms)
|
Builds ready [64d5613]
Page Load Metrics (606 ± 47 ms)
|
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/42