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

Modify parsing of response for Connect unary requests #668

Merged
merged 12 commits into from
Jun 7, 2023

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Jun 5, 2023

This modifies the response parsing for Connect unary requests based on the serialization format. If useBinaryFormat is true, it will use the output type's fromBinary against response.arrayBuffer.

If the serialization format is JSON, it will use the output type's fromJson method against response.json.

Note that this fixes two outstanding issues:

  • It allows React-Native to be used with Connect unary requests without the need for patches. See Add custom transport for React-Native examples-es#602 for more information. With this change in place, the React-Native example in Connect-ES Integration works over Connect and unary. Note that streaming still is out of scope.

  • This also fixes the hidden issues with Svelte and universal SSR. Tests were run with the Svelte example in Connect-ES Integration using this change and universal SSR is working as intended:

No additional XHR requests issued from the browser

image

Hydration data in the document returned from the initial load call:

image

@smaye81 smaye81 requested a review from timostamm June 5, 2023 20:58
@smaye81
Copy link
Member Author

smaye81 commented Jun 5, 2023

@timostamm I saw some tests for Connect and JSON format already present in the crosstests folder, but I can add more if there's other scenarios you'd like to verify.

Copy link
Member

@timostamm timostamm left a comment

Choose a reason for hiding this comment

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

Now that we support a feature that relies on the specific behavior of calling Response#json, we need to add a test to make sure we do not regress. Can you add test that provides a mocked fetch to the transport?

packages/connect-web/src/connect-transport.ts Outdated Show resolved Hide resolved
@smaye81
Copy link
Member Author

smaye81 commented Jun 6, 2023

Now that we support a feature that relies on the specific behavior of calling Response#json, we need to add a test to make sure we do not regress. Can you add test that provides a mocked fetch to the transport?

Sure. Do you want the mocked fetch to do anything special, or just delegate to the global fetch? They both have to return a Promise<Response>, so I'm not sure if we want to mock out any special circumstances?

@timostamm
Copy link
Member

The fetch passed to the transport has to assert that json() is called, and not arrayBuffer(). I'm not sure what the best solution is, but I imagine one way to do it is to override arrayBuffer() to throw an error. For good measure, override json() to change a variable that can be verified.

@smaye81
Copy link
Member Author

smaye81 commented Jun 6, 2023

@timostamm lmk what you think of this approach. I went with Jasmine spies because it seemed a lot easier to reason about.

It's a bit tough to use the spies for the Response object because the individual tests don't currently know the type of transport or options used (i.e. whether useBinaryFormat is true or false). So, I modified the list of crosstests to provide this information. This could probably be organized better, but I wanted to get your thoughts first.

@timostamm
Copy link
Member

It's a bit tough to use the spies for the Response object because the individual tests don't currently know the type of transport or options used (i.e. whether useBinaryFormat is true or false). So, I modified the list of crosstests to provide this information. This could probably be organized better, but I wanted to get your thoughts first.

There is a simple solution for this problem: Don't use describeTransports. Pushed up 6369528 as an example.

You can also avoid using a server completely. Pushed up c236657 as an example. This one also covers error responses. Errors are always JSON, even when the request was made with the binary format. This is easily missed, so we should cover it.

Please use the examples as you see fit, but:

  • Let's not put these tests into the crosstests directory: The directory is an implementation of the test cases defined in bufbuild/connect-crosstest, and we can't just add one.
  • Let's also not complicate the crosstests by providing the protocol a transport uses. The goal of the crosstest is to prove independency of the protocol. describeTransportsExcluding is a sufficient escape hatch for cases like HTTP/1.1 and full-duplex.

@smaye81
Copy link
Member Author

smaye81 commented Jun 7, 2023

Much simpler, thank you. Updated the PR.

@smaye81 smaye81 merged commit 61a43ae into main Jun 7, 2023
@smaye81 smaye81 deleted the sayers/modify_parse branch June 7, 2023 15:28
@smaye81 smaye81 mentioned this pull request Jun 14, 2023
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