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

Passing configuration options to JSON stream middleware #223

Merged
merged 4 commits into from Oct 7, 2022

Conversation

jpuri
Copy link
Contributor

@jpuri jpuri commented Sep 23, 2022

PR should be merged only after MetaMask/json-rpc-middleware-stream#27 is deployed.

json-rpc-middleware-stream takes care to replay DAPP action when reconnection message is received. This PR passes the configuration parameter for the replay message in json-rpc-middleware-stream.

@jpuri jpuri marked this pull request as ready for review October 5, 2022 17:28
@jpuri jpuri requested a review from a team as a code owner October 5, 2022 17:28
@jpuri jpuri requested review from Gudahtt and naugtur October 5, 2022 17:29
@mcmire
Copy link
Contributor

mcmire commented Oct 5, 2022

It looks like the new version of json-rpc-middleware-stream drops support for Node 12, upgrading the engines field to >=14.0.0. We would need to do the same thing for this library to be able to use this version.

@jpuri
Copy link
Contributor Author

jpuri commented Oct 6, 2022

PR is blocked by #225

@jpuri
Copy link
Contributor Author

jpuri commented Oct 7, 2022

The PR is ready for review @mcmire , @Gudahtt

@@ -71,7 +71,9 @@ export abstract class AbstractStreamProvider extends BaseProvider {
);

// Set up RPC connection
this._jsonRpcConnection = createStreamMiddleware();
this._jsonRpcConnection = createStreamMiddleware({
retryOnMessage: 'METAMASK_EXTENSION_STREAM_CONNECT',
Copy link
Member

Choose a reason for hiding this comment

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

Where is this message sent? Just trying to understand the whole solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message is sent from contentScript when extension stream is connected https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/contentscript.js#L395

@jpuri jpuri requested a review from Gudahtt October 7, 2022 17:01
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!

@jpuri jpuri merged commit 51d66de into main Oct 7, 2022
@jpuri jpuri deleted the middleware_config branch October 7, 2022 17:37
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.

None yet

3 participants