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

feat(jsonrpc): require Sync on transport params #474

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

clint419
Copy link
Contributor

No description provided.

@xJonathanLEI
Copy link
Owner

Can you explain how adding more constraints is improving flexibility? Also please don't submit PR without any description unless it's something very obvious like typo fixing. Thanks.

@clint419
Copy link
Contributor Author

Can you explain how adding more constraints is improving flexibility? Also please don't submit PR without any description unless it's something very obvious like typo fixing. Thanks.

Sorry, Purpose Used to customize JsonRpcTransport for extend, you can see ethers-rs, Thanks

@xJonathanLEI
Copy link
Owner

I guess I wasn't clear enough. I was asking for a specific example of what use case this enables. Like what scenario is this change enabling? What wasn't possible before this change and made possible thanks to the proposed changes?

@clint419
Copy link
Contributor Author

Hi~, @xJonathanLEI

I have added a transport for read and write separation(RwTransport), you can have a look at my sample, and later, according to more usage scenarios of our company, add more transport, such as RetryTransport, FailoverTransport and so on. Thanks.

@clint419
Copy link
Contributor Author

clint419 commented Sep 27, 2023

@xJonathanLEI
Hi~, I have added for RwTransport and FailoverTransport, you can look at the commit code. Thanks

@clint419
Copy link
Contributor Author

clint419 commented Sep 27, 2023

I guess I wasn't clear enough. I was asking for a specific example of what use case this enables. Like what scenario is this change enabling? What wasn't possible before this change and made possible thanks to the proposed changes?

Add constraints, you can implement more Transport, otherwise, the library user can not customize. usage scenario, such as read/write separation, fault tolerance and so on.

@xJonathanLEI xJonathanLEI force-pushed the support_multi_transports branch from 57d62e8 to 292515c Compare October 30, 2023 08:35
@xJonathanLEI
Copy link
Owner

@clint419 Thanks for the update. I've reviewed the use case and agree that having Sync on P would be great as otherwise types built on JsonRpcTransport might need to unnecessarily clone it.

I've updated the PR to only include the changes on the trait though. The additional transport impls have been removed. It's possible to implement those outside of the library.

@xJonathanLEI xJonathanLEI changed the title feat: Improve transport flexibility feat(jsonrpc): require Sync on transport params Oct 30, 2023
@xJonathanLEI
Copy link
Owner

This is a breaking change, but we're about to release a new version with many other breaking changes anyways. So we're good. Also this mostly only breaks applications that implement custom transports, and it's trivial to patch.

Copy link
Owner

@xJonathanLEI xJonathanLEI left a comment

Choose a reason for hiding this comment

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

LGTM

@xJonathanLEI xJonathanLEI merged commit d53338b into xJonathanLEI:master Oct 30, 2023
23 checks passed
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.

2 participants