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

Move stream functionality from BaseProvider to new StreamProvider #209

Merged
merged 20 commits into from
Jun 8, 2022

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Jun 5, 2022

The constructor of the BaseProvider expected a duplex stream, and the class as a whole contained a considerable amount of logic specific to streams and stream handling. This PR extracts this and other dependent functionality into a new class, StreamProvider, which is now extended by MetaMaskInpageProvider, while BaseProvider is made abstract. This will be useful for replacing internal legacy providers (which do not always / ever use streams) with EIP-1193 equivalents throughout the MetaMask codebase.

This PR should be completely non-breaking except for the changes made to the BaseProvider class. In addition, createExternalExtensionProvider, MetaMaskInpageProvider, and initializeProvider should have no behavior changes at all.

In detail, this PR:

  • Converts BaseProvider to an abstract class, and moves all stream-specific functionality into a new class, StreamProvider
  • Moves BaseProvider tests to the StreamProvider tests, adds an initialization test case
  • Adds optional rpcMiddleware parameter to BaseProvider
    • Middlewares previously instantiated and pushed inside of BaseProvider are now instead passed as constructor parameters to StreamProvider and MetaMaskInpageProvider as necessary.
  • Moves networkVersion-related logic from the BaseProvider to MetaMaskInpageProvider, while continuing to rely on the networkVersion having the value of 'loading' to detect when we are mid-network change in the StreamProvider (and by extension its child classes).
  • Updates the Jest config, adds jest-it-up and minimum coverage requirements

@rekmarks rekmarks force-pushed the base-provider-mk2 branch 4 times, most recently from 88fcf08 to cf3be4b Compare June 5, 2022 20:37
@rekmarks rekmarks force-pushed the base-provider-mk2 branch 3 times, most recently from fae748d to f5889b7 Compare June 6, 2022 00:03
src/StreamProvider.test.ts Outdated Show resolved Hide resolved
Base automatically changed from restructure-tests to main June 6, 2022 01:11
@rekmarks rekmarks marked this pull request as ready for review June 6, 2022 01:12
@rekmarks rekmarks requested a review from a team as a code owner June 6, 2022 01:12
mcmire
mcmire previously approved these changes Jun 6, 2022
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This is something that we've obviously been thinking about for a while and have wanted to do, so thank you for doing this. I took another look at EIP-1193 to verify that BaseProvider satisfies the minimum requirements and didn't see anything that stands out. I also like how minimal BaseProvider is now. A few questions/suggestions below, but overall looks good to me.

.github/workflows/build-test-lint.yml Outdated Show resolved Hide resolved
jest.config.js Outdated Show resolved Hide resolved
src/BaseProvider.ts Outdated Show resolved Hide resolved
src/BaseProvider.ts Show resolved Hide resolved
src/StreamProvider.ts Outdated Show resolved Hide resolved
src/StreamProvider.ts Outdated Show resolved Hide resolved
src/StreamProvider.test.ts Show resolved Hide resolved
src/StreamProvider.ts Outdated Show resolved Hide resolved
@rekmarks rekmarks requested a review from mcmire June 7, 2022 05:40
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

One more thing, then I'm good!

By the way one thing I've noticed is that you've changed all of the default exports to named exports. That isn't the first time I've seen that pattern, so I'm curious — what are your thoughts? What are the benefits you see using named exports vs. default exports? Are there any scenarios in which we would still want to use default exports?

src/BaseProvider.ts Show resolved Hide resolved
@rekmarks
Copy link
Member Author

rekmarks commented Jun 8, 2022

@mcmire, regarding named vs default exports, I think default exports make sense when you have a module that exports a single thing for which the name isn't that important. As soon as you are exporting multiple things from the same file, it's probably time for named exports. Mixing default and named exports isn't really that harmful in TypeScript IMO, but there's not really any benefit either. In JavaScript, I've found the practice to be actively harmful and confusing, because it's easy to mess up your imports and/or exports when your editor doesn't tell you about it.

The reason I made the change in this PR is because I made a change that caused a file to export two things instead of one, and then I just went through and removed all default exports except one, which is an internal log / error message utility (src/messages.ts).

On second thought, default exports also have the weakness of being importable under different names, which can be very confusing in large code bases. For our single message utility it doesn't matter, but it's been a real pain in the ass to deal with in the extension historically.

@mcmire
Copy link
Contributor

mcmire commented Jun 8, 2022

Okay, thanks for the explanation, makes sense!

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Two more things!

src/StreamProvider.ts Outdated Show resolved Hide resolved
src/StreamProvider.ts Show resolved Hide resolved
mcmire
mcmire previously approved these changes Jun 8, 2022
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks merged commit 235551b into main Jun 8, 2022
@rekmarks rekmarks deleted the base-provider-mk2 branch June 8, 2022 21:11
rekmarks added a commit that referenced this pull request Jun 9, 2022
I accidentally mangled the docstring in #209. This PR fixes it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants