-
-
Notifications
You must be signed in to change notification settings - Fork 186
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
fix(gas-fee-controller): add missing @metamask/polling-controller #1748
fix(gas-fee-controller): add missing @metamask/polling-controller #1748
Conversation
efd31f3
to
65d0632
Compare
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
65d0632
to
128ed9c
Compare
f6875b4
to
2ea2909
Compare
2ea2909
to
46d975f
Compare
@@ -55,7 +56,8 @@ | |||
"typescript": "~4.8.4" | |||
}, | |||
"peerDependencies": { | |||
"@metamask/network-controller": "^13.0.1" | |||
"@metamask/network-controller": "^13.0.1", | |||
"@metamask/polling-controller": "^0.0.0" |
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.
Welp. This shouldn't be a peerDependency in this case, but the contraints are enforcing it due to the name.
The intention was to enforce that controllers are peerDependencies of each other. But the polling-controller
package is not itself a controller, it's a mixin meant to be used by controllers.
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.
Not sure how to proceed here @Gudahtt ?
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.
We can add an exception for polling-controller
to the constraints here:
Line 308 in a1e1883
DependencyIdent \= '@metamask/eth-keyring-controller', |
We've already added an exception for two other packages
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.
Ok I'll add that in this PR.
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.
done here: fb4260a
f68de2e
to
506ef4b
Compare
506ef4b
to
fb4260a
Compare
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!
) ## Explanation Adds missing dependency introduced in #1673 ## References - Follow-up to: #1673 ## Changelog ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex <adonesky@gmail.com>
) ## Explanation Adds missing dependency introduced in #1673 ## References - Follow-up to: #1673 ## Changelog ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex <adonesky@gmail.com>
) ## Explanation Adds missing dependency introduced in #1673 ## References - Follow-up to: #1673 ## Changelog ## Checklist - [ ] I've updated the test suite for new or updated code as appropriate - [ ] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [ ] I've highlighted breaking changes using the "BREAKING" category above as appropriate --------- Co-authored-by: Alex <adonesky@gmail.com>
Explanation
Adds missing dependency introduced in #1673
References
Changelog
Checklist