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.

Sign up for GitHub

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

Jump to bottom

Update local client to support middleware (Kudos Seun) #589

Merged
merged 10 commits into from
Sep 30, 2020
Merged

Update local client to support middleware (Kudos Seun) #589

merged 10 commits into from
Sep 30, 2020

Conversation

tomusdrw
Copy link
Contributor

@tomusdrw tomusdrw commented Sep 25, 2020 •
edited by seunlanlege
Loading

Previously the jsonrpc_core_client::transports::local::connect consumed a Deref<Target=MetaIoHandler<TMetadata, NoopMiddleware>>,

This PR corrects that behaviour allowing the local transport support other Middleware implementations

@tomusdrw tomusdrw changed the title v15.0.1 v15.0.1 - update local client to support metadata (Kudos Seun) Sep 25, 2020
dvdplm
dvdplm reviewed Sep 25, 2020
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

A PR description would be good.

This adds a new type parameter; isn't that a breaking change?

seunlanlege
seunlanlege previously approved these changes Sep 25, 2020
View reviewed changes
tomusdrw
tomusdrw commented Sep 25, 2020
View reviewed changes
core-client/transports/src/transports/local.rs Outdated
@@ -98,10 +102,11 @@ where
}

/// Connects to a `Deref<Target = MetaIoHandler<Metadata + Default>`.
pub fn connect<TClient, THandler, TMetadata>(handler: THandler) -> (TClient, impl Future<Item = (), Error = RpcError>)
pub fn connect<TClient, THandler, TMetadata, TMiddleware>(handler: THandler) -> (TClient, impl Future<Item = (), Error = RpcError>)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @dvdplm noticed it's a breaking change (so would require bumping to 16), perhaps better to add a new set of methods like connect_with_middleware or set the default type somehow?
Usage of middlewares is not super common, so I think it's better to keep the default connect stuff simple.

@tomusdrw tomusdrw changed the title v15.0.1 - update local client to support metadata (Kudos Seun) Update local client to support middleware (Kudos Seun) Sep 25, 2020
niklasad1
niklasad1 reviewed Sep 25, 2020
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Is it reasonable to merge with master instead after the breaking changes? I think there is some unreleased stuff there (futures v0.3) IIRC.

@seunlanlege
Copy link
Contributor

any luck this can be merged and released today?

@tomusdrw
Copy link
Contributor Author

@seunlanlege Please see #589 (comment)
This is a breaking change it either requires bump to 16.0 or we must introduce it in a backward compatible way.

@seunlanlege
Copy link
Contributor

seunlanlege commented Sep 28, 2020 •
edited
Loading

Weird I thought it could be released under 15.1? Either way, bumping to 16 feels extreme. Adding a new method, connect_with_middleware feels more optimal

niklasad1
niklasad1 reviewed Sep 29, 2020
View reviewed changes
core-client/transports/src/transports/local.rs Outdated
}

/// Connects to a `Deref<Target = MetaIoHandler<Metadata + Default>`.
pub fn connect<TClient, THandler, TMetadata>(
Copy link
Member

Choose a reason for hiding this comment

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

hrm, this will connect without Middleware (Noop) because of MetaIoHandler<TMetadata>? It seems unintuitive to me but I understand that it wouldn't be backward compatible otherwise. Perhaps explain this with better documentation and remove it in v16 later?

Copy link
Contributor

Choose a reason for hiding this comment

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

it will connect with the NoopMiddleware

Copy link
Member

@niklasad1 niklasad1 Sep 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yepp, that was what I meant with Middleware (Noop), but I was mostly referring to:

Connects to a Deref<Target = MetaIoHandler<Metadata + Default>

It's not easy to understand that it's using the default associated type NoopMiddleware at least not to me but I guess it could be deduced by the other with_* methods.

niklasad1
niklasad1 reviewed Sep 29, 2020
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

@seunlanlege

The repo is using cargo fmt please use it and the CI will pass.

niklasad1
niklasad1 previously approved these changes Sep 29, 2020
View reviewed changes
niklasad1
niklasad1 requested changes Sep 29, 2020
View reviewed changes
Copy link
Member

@niklasad1 niklasad1 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The PR is now mixing versions 15.1.0 and 15.0.1 which causes the build to fail.

IMHO, we should bump minor because it actually adds functionality in backward-compatible manner. If you don't agree please convince me :)

EDIT:
use ./_automate/bump_version.sh to bump versions

tomusdrw
tomusdrw commented Sep 30, 2020
View reviewed changes
Copy link
Contributor Author

@tomusdrw tomusdrw left a comment

Choose a reason for hiding this comment

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

lgtm!

@dvdplm dvdplm merged commit ca1fc86 into v15.x Sep 30, 2020
@dvdplm dvdplm deleted the v15.0.1 branch September 30, 2020 09:09
tomusdrw added a commit that referenced this pull request Dec 9, 2020
@tomusdrw @seunlanlege @niklasad1
Update local client to support middleware (Kudos Seun) (#589)
7f77590 
* v15.0.1

* v15.0.1 => v15.1.0

* v15.0.1 => v15.1.0

* cargo fmt

* fix tests

* adds *_with_middleware methods

* remove Unpin bound, add documentation, cargo fmt

* 15.0.1

* bump to 15.1.0

Co-authored-by: Seun Lanlege <seunlanlege@gmail.com>
Co-authored-by: Niklas <niklasadolfsson1@gmail.com>
dvdplm pushed a commit that referenced this pull request Dec 9, 2020
@tomusdrw @seunlanlege @niklasad1
[forwardport] Update local client to support middleware (Kudos Seun) (
61ea7c6 
#589) (#600)

* Update `local` client to support middleware (Kudos Seun) (#589)

* v15.0.1

* v15.0.1 => v15.1.0

* v15.0.1 => v15.1.0

* cargo fmt

* fix tests

* adds *_with_middleware methods

* remove Unpin bound, add documentation, cargo fmt

* 15.0.1

* bump to 15.1.0

Co-authored-by: Seun Lanlege <seunlanlege@gmail.com>
Co-authored-by: Niklas <niklasadolfsson1@gmail.com>

* cargo fmt --all

Co-authored-by: Seun Lanlege <seunlanlege@gmail.com>
Co-authored-by: Niklas <niklasadolfsson1@gmail.com>
This was referenced Mar 12, 2021
Merged
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seunlanlege seunlanlege seunlanlege left review comments

@dvdplm dvdplm dvdplm left review comments

@niklasad1 niklasad1 niklasad1 approved these changes

@maciejhirsz maciejhirsz maciejhirsz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tomusdrw @seunlanlege @dvdplm @maciejhirsz @niklasad1