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

Streaming API #31

Merged
merged 2 commits into from
Jun 13, 2015
Merged

Streaming API #31

merged 2 commits into from
Jun 13, 2015

Conversation

clue
Copy link
Owner

@clue clue commented Mar 16, 2015

First draft. API is subject to change in future versions.

Closes #26.

@asm89
Copy link
Contributor

asm89 commented Mar 17, 2015

👍 API looks a little bit clunky, but it seems like the best way given the promise API.

@@ -122,6 +122,8 @@ public function send(Request $request)
));
}
});

$deferred->progress(array('responseStream' => $response, 'requestStream' => $requestStream));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: have constants defined on Sender instead of these strings? (Part of public API.)

@clue
Copy link
Owner Author

clue commented Jun 13, 2015

👍 API looks a little bit clunky

I'm not even going to argue on this :-) Thanks for your feedback, much appreciated!

I'd like to get this in rather sooner than later (because other libs depend on this feature).

I'm not a fan of the API, hence the statement in the README. I'm looking forward to introduce a breaking change as soon as we figure out a better API 👍

clue added a commit that referenced this pull request Jun 13, 2015
@clue clue merged commit c69642b into clue:master Jun 13, 2015
@clue clue deleted the stream branch June 13, 2015 18:37
@clue
Copy link
Owner Author

clue commented Jun 30, 2015

For the reference: This API is subject to change because it takes advantage of the dreaded promise progress API (reactphp/promise#32). The fetch standard API (#30) might be a good alternative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support streaming responses
2 participants