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

Support fetch api request stream #670

Closed
wants to merge 3 commits into from
Closed

Conversation

oott123
Copy link

@oott123 oott123 commented Jun 6, 2023

This fixes #669 for connect transport. Maybe we should support it for grpc transport too.

@CLAassistant
Copy link

CLAassistant commented Jun 6, 2023

CLA assistant check
All committers have signed the CLA.

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.

Thanks for the PR, @oott123!

As mentioned in #669 (comment), we'll need quite a bit more plumbing to get this merged though, especially regarding test coverage. We are kindly sponsored by the BrowserStack Open Source program, so it should be possible to get good confidence with tests for the happy path and the unhappy paths.

packages/connect-web/src/connect-transport.ts Outdated Show resolved Hide resolved
@oott123 oott123 marked this pull request as draft June 7, 2023 16:36
@oott123 oott123 marked this pull request as ready for review June 7, 2023 17:04
@oott123
Copy link
Author

oott123 commented Jun 7, 2023

I have added crosstests which taken from nodejs client streaming.

However I need help on how to write test spec if we are expected to fail on some browsers.

@timostamm
Copy link
Member

@oott123, let me wrap up #673 first, I will take a look then.

@timostamm
Copy link
Member

Apologies that this hasn't gotten attention yet, @oott123. Hope we can help push this across soon.

@timostamm
Copy link
Member

I got pulled into other directions, apologies.

Unfortunately, Safari and Firefox still do not implement the feature. We have a more comprehensive test suite now that allows us to more comprehensively test client streams, but it doesn't run on old browsers yet (see #810).

So this feature is blocked on two sides: Our test coverage isn't sufficient yet, and browsers don't support it across the board. This means that we cannot provide this feature yet.

Once Safari and Firefox have caught up, we can revisit.

@timostamm timostamm closed this Sep 11, 2024
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.

Support Client Streaming for fetch API
3 participants