refactor(StreamProvider): remove unnecessary stream multiplexing#400
Conversation
cryptodev-2s
force-pushed
the
cryptodev2s/refactor-providers-stream-connection
branch
from
d936335 to
745d3c5
Compare
Gudahtt
left a comment
Gudahtt
left a comment
There was a problem hiding this comment.
Looks good! Just one last requested change for the inpage provider options.
It would be great to see the changelog entries before approving as well
| let warningMsg = `Lost connection to "${jsonRpcStreamName}".`; | ||
| if (error?.stack) { | ||
| warningMsg += `\n${error.stack}`; | ||
| } |
There was a problem hiding this comment.
thoughts on just stringifying and logging the entire error object?
There was a problem hiding this comment.
I tried to use the same pattern that was used earlier and as still in here https://github.com/MetaMask/providers/blob/main/src/StreamProvider.ts#L155
But I haven't nothing against making this change
|
Why not move the stream multiplexing outside of this package entirely? I ask because I have a POC branch for adding Multichain API support over WindowPostMessage that requires me to instantiate yet another ObjectMultiplex and substream outside of the inpage provider instantiator. It seems to make more sense and is cleaner if we just remove this substream creation responsibility from these providers. I.e. the providers should just operate off the original Duplex Stream instance given to them and that's it |
|
@jiexi I had the same thought. I'm not 100% sure how I feel about the idea, but agreed that it's worth considering. However I still see this PR as a step towards that. Extracting the steam multiplexing completely has some potential downsides, but this step does not (as far as I can tell). |
cryptodev-2s
deleted the
cryptodev2s/refactor-providers-stream-connection
branch
3 participants
Eliminate redundant stream multiplexing within
StreamProviderand move the multiplexing logic to the consumer side.Changelog: See changelog changes