refactor!: Replace SafeEventEmitterProvider with InternalProvider
#6796
Conversation
rekmarks
changed the title
SafeEventEmitterProvider with InternalProvider
There was a problem hiding this comment.
I think there's one thing with the proxies that we may want to adjust, but the rest looks good. Thank you for removing the lint warnings as well!
Can you:
- generate preview builds
- make a new branch on Extension, upgrade
network-controllerto the preview build, push the PR, and wait for CI to pass - build the extension locally, open the test dapp, and make sure you can still make network requests on different chains
- do the same thing for mobile
Let me know if you want to tag team this, I have a bit of time now.
packages/selected-network-controller/src/SelectedNetworkController.ts
Outdated
Show resolved
Hide resolved
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
cc: @mcmire |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
rekmarks
force-pushed
the
rekm/internal-provider
branch
2 times, most recently
from
c48651b to
2caa429
Compare
Co-authored-by: Elliot Winkler <[email protected]>
rekmarks
force-pushed
the
rekm/internal-provider
branch
from
2caa429 to
f0dbedf
Compare
| * @deprecated Use {@link InternalProvider} instead. | ||
| */ | ||
| type SafeEventEmitterProvider = InternalProvider; | ||
| const SafeEventEmitterProvider = InternalProvider; |
There was a problem hiding this comment.
What is the reasoning for keeping the alias around if we are removing it everywhere?
There was a problem hiding this comment.
Builds / tests fail because something monorepo-external uses SafeEventEmitterProvider and removing it completely blows up. My suspicion is that all library uses of it need to be removed, then we can delete the deprecated alias completely.
|
|
||
| this.messenger.registerActionHandler( | ||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions |
There was a problem hiding this comment.
Why are all of these no longer required? 🤔
There was a problem hiding this comment.
My suspicion is that they weren't required prior to this PR, and the manner in which I linted the package (yarn eslint --fix packages/... from root) removed a bunch of unused disable directives.
There was a problem hiding this comment.
When these lint violations appeared, at the time we were using an older version of the typescript-eslint packages. When we upgraded, they went away.
| import { type JsonRpcRequest, type Json } from '@metamask/utils'; | ||
| import { BrowserProvider } from 'ethers'; | ||
| import { promisify } from 'util'; | ||
| // eslint-disable-next-line import-x/namespace |
There was a problem hiding this comment.
Ugh, I'm not sure why we have this lint rule, it doesn't make sense to me. Oh well...
|
|
||
| this.messenger.registerActionHandler( | ||
| // TODO: Either fix this lint violation or explain why it's necessary to ignore. | ||
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions |
There was a problem hiding this comment.
When these lint violations appeared, at the time we were using an older version of the typescript-eslint packages. When we upgraded, they went away.
|
I am bypassing rules to merge this without a review from @MetaMask/confirmations. Their review requirement was triggered by changes to |
Explanation
As of #6328
SafeEventEmitterProviderno longer used theEventEmitterAPI, making its name misleading. Consequently, this PR renames it toInternalProviderand stops extendingSafeEventEmitter. This new name better reflects its use as an internal-only provider that does not conform to any particular standard.SafeEventEmitterProvideris still exported as a deprecated alias ofInternalProviderin order to circumvent build failures due to repo-external packages that use this alias. Once we have updated external packages to useInternalProviderinstead, the old alias can be removed.References
Closes #6594
Checklist
Note
Renames and replaces SafeEventEmitterProvider with InternalProvider across packages, updates proxies and types, adds a deprecated alias, and adjusts tests/changelogs accordingly.
InternalProvider(no longer extends SafeEventEmitter); updateproviderFromEngine/providerFromMiddlewareto return it.SafeEventEmitterProvideras a deprecated alias; update tests/exports; drop@metamask/safe-event-emitterdep.InternalProviderinblock-ref,retryOnEmpty, andproviderAsMiddleware.InternalProviderinPollingBlockTrackerand tests; update CHANGELOG (BREAKING).InternalProvideracross types/clients.createSwappableProxyfor provider proxies; keepcreateEventEmitterProxyfor block tracker; adjust handlers/comments and tests.createSwappableProxyfor provider proxy; align withInternalProvider; update tests.FakeProvider/FakeBlockTrackerand various tests toInternalProvider.Written by Cursor Bugbot for commit e1faf35. This will update automatically on new commits. Configure here.