-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
MV3: add retry logic to actions #15337
Conversation
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. |
Progresses #14853 |
5770f82
to
a880ead
Compare
Builds ready [4d41431]
Page Load Metrics (1831 ± 266 ms)
highlights:storybook
|
ui/store/action-queue/index.test.js
Outdated
// connectionStream: { | ||
// readable: true, | ||
// }, | ||
// backgroundFunction: async () => { |
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.
this was supposed to be a callback style function - it's being wrapped in pify afterall.
Hours spent here: 2 :D
Currently when clearing the queue we run action asynchronously, but I am thinking may be we should do it synchronous as they are in sequence of user interaction. Consider user updating gas params on transaction twice - we need to ensure that they run in sequence. |
I thought executing them quickly was a requirement. This makes implementation much nicer, may even help eliminate the need for the splice. |
Builds ready [0c19e3e]
Page Load Metrics (1828 ± 85 ms)
highlights:storybook
|
Is this a feature branch? Is the idea here that all those idempotent prs are gonna collect here? Could we add the PR template here and fill it out with as much detail as possible |
Hey @brad-decker : the intend is not to make it feature branch, this is ready for merge. Other PRs are based on this as they need these changes to work. |
Co-authored-by: Ariella Vu <20778143+digiwand@users.noreply.github.com>
@@ -11,7 +11,7 @@ import MetaMetricsProviderStorybook from './metametrics'; | |||
import testData from './test-data.js'; | |||
import { Router } from 'react-router-dom'; | |||
import { createBrowserHistory } from 'history'; | |||
import { _setBackgroundConnection } from '../ui/store/actions'; | |||
import { _setBackgroundConnection } from '../ui/store/action-queue'; |
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.
NIT: my brain sees underscore before a variable/method and it thinks private
variable/method.
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 would avoid changing existing method in this PR.
Builds ready [8161b91]
Page Load Metrics (1735 ± 56 ms)
highlights:storybook
|
Builds ready [b67442c]
Page Load Metrics (1724 ± 46 ms)
highlights:storybook
|
Builds ready [678f4af]
Page Load Metrics (1878 ± 58 ms)
highlights:storybook
|
Builds ready [85c7075]
Page Load Metrics (1715 ± 29 ms)
highlights:storybook
|
Co-authored-by: Zbyszek Tenerowicz <naugtur@gmail.com>
Builds ready [650a94c]
Page Load Metrics (1786 ± 63 ms)
highlights:storybook
|
Builds ready [a17b32a]
Page Load Metrics (1865 ± 69 ms)
highlights:storybook
|
… mv3_action_retry
Builds ready [cebe284]
Page Load Metrics (1639 ± 30 ms)
highlights:storybook
|
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.
LGTM! The only thing I saw that I questioned was the big comment block in action-queue/index.js
; do we need that?
This is great!
Fixes: #15535
Add retry logic to actions