grpc-web-devtools icon indicating copy to clipboard operation
grpc-web-devtools copied to clipboard

Published 20 hours ago • verified publisher icon SafetyCulture

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

Open timostamm opened this issue 1 year ago • 4 comments

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 https://github.com/SafetyCulture/grpc-web-devtools/pull/160 and https://github.com/SafetyCulture/grpc-web-devtools/pull/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

  • [x] 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.

timostamm avatar Aug 07 '23 11:08 timostamm

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

vonsky104 avatar Jan 05 '24 11:01 vonsky104

@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.

matt-lewandowski avatar Jan 08 '24 03:01 matt-lewandowski

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?

timostamm avatar Jan 08 '24 17:01 timostamm

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

matt-lewandowski avatar Jan 08 '24 23:01 matt-lewandowski