Skip to content
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

Merged

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Oct 2, 2023

Explanation

Adds missing dependency introduced in #1673

References

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

@legobeat legobeat force-pushed the deps-gas-controller-add-polling-controller branch from efd31f3 to 65d0632 Compare October 2, 2023 10:52
@legobeat legobeat added bug Something isn't working dependencies Pull requests that update a dependency file labels Oct 2, 2023
@legobeat legobeat changed the title fix(gas-fee-controler): add missing @metamask/polling-controller fix(gas-fee-controller): add missing @metamask/polling-controller Oct 2, 2023
@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/polling-controller 0.0.0 None +0 3.37 kB metamaskbot

@legobeat legobeat force-pushed the deps-gas-controller-add-polling-controller branch from 65d0632 to 128ed9c Compare October 2, 2023 10:55
@legobeat legobeat mentioned this pull request Oct 2, 2023
@legobeat legobeat force-pushed the deps-gas-controller-add-polling-controller branch 2 times, most recently from f6875b4 to 2ea2909 Compare October 2, 2023 11:01
@legobeat legobeat marked this pull request as ready for review October 2, 2023 11:06
@legobeat legobeat requested a review from a team as a code owner October 2, 2023 11:06
@legobeat legobeat force-pushed the deps-gas-controller-add-polling-controller branch from 2ea2909 to 46d975f Compare October 2, 2023 11:17
@legobeat legobeat requested a review from OGPoyraz October 2, 2023 12:23
OGPoyraz
OGPoyraz previously approved these changes Oct 2, 2023
@@ -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"
Copy link
Member

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.

Copy link
Contributor

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 ?

Copy link
Member

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:

DependencyIdent \= '@metamask/eth-keyring-controller',

We've already added an exception for two other packages

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done here: fb4260a

@adonesky1 adonesky1 force-pushed the deps-gas-controller-add-polling-controller branch from 506ef4b to fb4260a Compare October 2, 2023 17:45
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@adonesky1 adonesky1 merged commit 94175cd into MetaMask:main Oct 2, 2023
107 checks passed
@legobeat legobeat deleted the deps-gas-controller-add-polling-controller branch October 2, 2023 20:16
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
)

## 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>
MajorLift pushed a commit that referenced this pull request Oct 11, 2023
)

## 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>
MajorLift pushed a commit that referenced this pull request Oct 12, 2023
)

## 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants