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

Update connect-web-interceptor.js for v0.8.0 and later #166

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timostamm
Copy link

Summary

Update the connect-web-interceptor.js to be compatible with @buildbuild/connect-web >= v0.8.0 for server-streaming RPCs.

Problem description

Release v0.8.0 of @buildbuild/connect-web made a breaking change to interceptors. Unary RPCs are still compatible, but server-streaming RPCs no longer show up in the grpc-web-devtools.

With this change, server-streaming RPCs show up in the grpc-web-devtools again.

Other changes:

  • Show the full RPC name in the devtools - for example connectrpc.eliza.v1.ElizaService/Say instead of just Say.
  • Use the JSON option emitDefaultValues for a more human-friendly representation.
  • Swallow JSON serialization errors that may occur when a google.protobuf.Any is present in the schema.
  • Fix the connect-web integration instructions, superseding fix connect-web integration instruction #160 and Fix connect-web snippet #155

Server-streaming and unary RPCs tested with Chrome and Firefox:

a

Errors in unary and server-streaming RPCs tested with Chrome and Firefox:

b c

Pros/cons of approach implemented

The behavior for users of versions < 0.8.0 changes with this PR - they will no longer see server-streaming RPCs in the grpc-web-devtools. Since less than 2% of the downloads for of @bufbuild/connect-web are for older versions, this

It would ideal to support type registries for google.protobuf.Any, but swallowing seems better than crashing.

Checklist

  • Is this PR a reasonable size?

Code Review Guidelines for Reviewers

  • Try to review in a timely manner. Opinions/nitpicks should not be blockers. Pair on a call for non-trivial feedback.
  • Overall design and approach should follow established patterns. Don't try to make the PR perfect.
  • Try to identify edge cases, race conditions, over-engineering, lack of test coverage and complexity.
  • If you don't feel qualified to review the code, pass it on to someone who is.

@vonsky104
Copy link

@matt-lewandowski Is this something that you could consider merging? That would help a lot :) Thanks in advance!

@matt-lewandowski
Copy link
Contributor

@matt-lewandowski Is this something that you could consider merging? That would help a lot :) Thanks in advance!

Hey @vonsky104 👋

It looks like this PR is outdated and the newer interceptor handles streaming for versions < 0.8.0.

@timostamm
Copy link
Author

Hey @matt-lewandowski, v0.8.0 is nearly a year old and we've cut a stable v1 since. Would you accept the PR if I update it?

@matt-lewandowski
Copy link
Contributor

Hey @matt-lewandowski, v0.8.0 is nearly a year old and we've cut a stable v1 since. Would you accept the PR if I update it?

Hey @timostamm 👋

I don't believe we have cut over to v1 just yet. Has there been more breaking changes for V1 or would these changes still just be targeting the v0.8.0 breaking changes? If so, I would be happy to accept the PR

@mziwisky
Copy link

It would ideal to support type registries for google.protobuf.Any, but swallowing seems better than crashing.

Just want to comment on this bit -- this would be great to have, regardless of the other changes here. If this PR is held up on other concerns, maybe we could break out the safe JSON parsing bit into its own PR and merge that?

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.

4 participants