Migrate CurrencyRateController to BaseControllerV2 #372
Conversation
This file contains 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
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
1ffff86
to
eb26f36
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
eb26f36
to
fa0d1b9
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
fa0d1b9
to
f34e6b6
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
f34e6b6
to
017c494
Compare
Gudahtt
force-pushed
the
replace-composable-controller
branch
from
747e9c3
to
f63c6f1
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
017c494
to
14b2f4f
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
14b2f4f
to
7d115f5
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
7d115f5
to
cd60734
Compare
Gudahtt
changed the base branch from
develop
to
composed-controller-v2-compatibility
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
2 times, most recently
from
11c6f30
to
d3a7adc
Compare
Gudahtt
force-pushed
the
composed-controller-v2-compatibility
branch
from
c38021a
to
6ee460f
Compare
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
d3a7adc
to
f83b388
Compare
Base automatically changed from
composed-controller-v2-compatibility
to
develop
April 22, 2021 21:49
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
5 times, most recently
from
15d67c8
to
e5bb8f4
Compare
The CurrencyRateController has been migrated to the new base controller. The configuration can now only be set during construction, as it was never changed at runtime in practice with the old controller. Similarly, the `disable` function was removed as it wasn't relied upon in practice. One major change is that the controller doesn't poll after being constructed. The "start" method must be called for polling to start. The "start" and "stop" methods have been added to loosely follow the conventions used in `metamask-extension` to allow starting and stopping polling for the purpose of reducing network traffic. A few tests required substantial updates because of this change. The ComposableController tests were easiest to fix by deleting the use of the CurrencyRateController completely, since that test was intended to test BaseController-based controllers specifically. The last TokenRatesController test managed to mark the state change handlers in that controller as 'covered' without actually asserting anything related to that functionality. That broken test was split into two pieces, one for each state change handler. Those two tests still rely upon mocking a would-be private function, which isn't great, but it matches the patterns used elsewhere in that test module. To improve it would take a great deal of work, enough for a separate PR.
Gudahtt
force-pushed
the
migrate-currency-rate-controller
branch
from
e5bb8f4
to
3947a30
Compare
rekmarks
reviewed
May 4, 2021
|
The |
rekmarks
requested changes
May 4, 2021
The `handle` private variable has been renamed to better reflect what it is. Also the type was updated to the more-specific `NodeJS.Timeout`. It seems a bit odd to call this a "timeout" given that it's for a repeating interval, but this is the term the Node.js docs use, and I can't think of a better one.
rekmarks
requested changes
May 5, 2021
They've been renamed to match the terms used in the MDN docs.
Merged
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The CurrencyRateController has been migrated to the new base controller. The configuration can now only be set during construction, as it was never changed at runtime in practice with the old controller. Similarly, the `disable` function was removed as it wasn't relied upon in practice. One major change is that the controller doesn't poll after being constructed. The "start" method must be called for polling to start. The "start" and "stop" methods have been added to loosely follow the conventions used in `metamask-extension` to allow starting and stopping polling for the purpose of reducing network traffic. A few tests required substantial updates because of this change. The ComposableController tests were easiest to fix by deleting the use of the CurrencyRateController completely, since that test was intended to test BaseController-based controllers specifically. The last TokenRatesController test managed to mark the state change handlers in that controller as 'covered' without actually asserting anything related to that functionality. That broken test was split into two pieces, one for each state change handler. Those two tests still rely upon mocking a would-be private function, which isn't great, but it matches the patterns used elsewhere in that test module. To improve it would take a great deal of work, enough for a separate PR.
MajorLift
pushed a commit
that referenced
this pull request
Oct 11, 2023
The CurrencyRateController has been migrated to the new base controller. The configuration can now only be set during construction, as it was never changed at runtime in practice with the old controller. Similarly, the `disable` function was removed as it wasn't relied upon in practice. One major change is that the controller doesn't poll after being constructed. The "start" method must be called for polling to start. The "start" and "stop" methods have been added to loosely follow the conventions used in `metamask-extension` to allow starting and stopping polling for the purpose of reducing network traffic. A few tests required substantial updates because of this change. The ComposableController tests were easiest to fix by deleting the use of the CurrencyRateController completely, since that test was intended to test BaseController-based controllers specifically. The last TokenRatesController test managed to mark the state change handlers in that controller as 'covered' without actually asserting anything related to that functionality. That broken test was split into two pieces, one for each state change handler. Those two tests still rely upon mocking a would-be private function, which isn't great, but it matches the patterns used elsewhere in that test module. To improve it would take a great deal of work, enough for a separate PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The CurrencyRateController has been migrated to the new base controller. The configuration can now only be set during construction, as it was never changed at runtime in practice with the old controller. Similarly, the
disablefunction was removed as it wasn't relied upon in practice.One major change is that the controller doesn't poll after being constructed. The "start" method must be called for polling to start. The "start" and "stop" methods have been added to loosely follow the conventions used in
metamask-extensionto allow starting and stopping polling for the purpose of reducing network traffic.A few tests required substantial updates because of this change. The ComposableController tests were easiest to fix by deleting the use of the CurrencyRateController completely, since that test was intended to test BaseController-based controllers specifically.
The last TokenRatesController test managed to mark the state change handlers in that controller as 'covered' without actually asserting anything related to that functionality. That broken test was split into two pieces, one for each state change handler. Those two tests still rely upon mocking a would-be private function, which isn't great, but it matches the patterns used elsewhere in that test module. To improve it would take a great deal of work, enough for a separate PR.