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

Grpc-web with metadata support #20

Closed
wants to merge 6 commits into from
Closed

Conversation

mineme0110
Copy link
Contributor

No description provided.

build.sbt Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
@mineme0110 mineme0110 force-pushed the master branch 2 times, most recently from ea83cde to 1ed77e3 Compare April 23, 2020 17:23
@thesamet
Copy link
Contributor

Can we generate both traits with metadata and without metadata traits (and omit the flag that chooses). This enables users to gradually migrate one interface at a time, rather than all at once. The traits should have two names: MyService and MyServiceWithMetadata.

@AlexITC
Copy link
Contributor

AlexITC commented Apr 28, 2020

@thesamet I wonder whether using a default argument could be enough to keep a single interface, so that existing code is not broken and you are free to use the metadata when you need it, while this breaks binary compatibility (from what I understand), I believe we don't need to worry about it this library as it's marked as experimental.

@thesamet
Copy link
Contributor

OK - let's do a default argument. I want to also another change to align with future intentions with scalapb grpc and present state in zio-grpc, the trait should be:

class MyService[A](f: A=>Metadata, defaultContext: =>A) {
  def unaryMethod(request: RequestType, Context: A=f(defaultContext))
}

We can have an helper that instantiates a client when A = Metadata and f is the identity function, but it helps user to deal with domain object up to the client call.

@AlexITC
Copy link
Contributor

AlexITC commented Apr 28, 2020

@thesamet the last suggested change seems to require a bit more work, would you mind if we do that in another PR? we would like to continue on the integration which depends on this PR.

@thesamet
Copy link
Contributor

I think that the additional work is pretty marginal - would be preferred over a quick followup with a breaking change.

@mineme0110 mineme0110 force-pushed the master branch 2 times, most recently from ce69a57 to bb5c9b9 Compare April 30, 2020 22:05
@thesamet
Copy link
Contributor

thesamet commented May 1, 2020

Is this ready for review or more changes planned?

@AlexITC
Copy link
Contributor

AlexITC commented May 1, 2020

@thesamet it's ready, your requested changes were applied.

Thanks.

@thesamet
Copy link
Contributor

thesamet commented May 2, 2020

Merged via 52f7fd4

@thesamet thesamet closed this May 2, 2020
@thesamet thesamet mentioned this pull request May 2, 2020
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