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

Rename Server to StreamingServer #197

Closed
WyriHaximus opened this issue May 30, 2017 · 6 comments
Closed

Rename Server to StreamingServer #197

WyriHaximus opened this issue May 30, 2017 · 6 comments
Assignees
Milestone

Comments

@WyriHaximus
Copy link
Member

During the discussion in #28 it came up that users might expect a PSR-7 request to be sync and with the body fully downloaded, parsed and ready to be used. The default Server in react/http this isn't the case. Thus renaming it to StreamingServer, making the Server workings more obvious, is IMHO the way to go.

@andig
Copy link
Contributor

andig commented May 30, 2017

Thus renaming it to StreamingServer, making the Server workings more obvious, is IMHO the way to go.

Wouldn't hurt, but on the other side everybody using react should meanwhile be aware that he's in async wonderland.

Would we be able to- instead of having two servers for sync and async- detect string context on a getBody() call and return the entire body, maybe wrapped in a promise, instead?

I wouldn't expect sync return values at all from this package but- especially for the client- it might be handy to return an entire body instead of in parts.

@WyriHaximus
Copy link
Member Author

Wouldn't hurt, but on the other side everybody using react should meanwhile be aware that he's in async wonderland.

True, very true. No I'm doubt whether we should rename or not, but that's what we have this issue for 😄 .

Would we be able to- instead of having two servers for sync and async- detect string context on a getBody() call and return the entire body, maybe wrapped in a promise, instead?

Effectively the server from #198 would wrap the Server/StreamingServer anyway with the ability to handle the request bodies with the streaming body parsers and a streaming body buffered sink. If you want more control over the body parsing you could use the body parsers your self or use the streaming body buffered sink your self. All that is to be able to do this:

I wouldn't expect sync return values at all from this package but- especially for the client- it might be handy to return an entire body instead of in parts.

And with sync values I more meant a complete ready to use PSR-7 request, as multiple people expressed their concerns about in #28 .

@clue
Copy link
Member

clue commented May 30, 2017

Thus renaming it to StreamingServer, making the Server workings more obvious, is IMHO the way to go.

Wouldn't hurt, but on the other side everybody using react should meanwhile be aware that he's in async wonderland.

I concur, I'm not opposed, but how much would be gained by this BC break?

Would we be able to- instead of having two servers for sync and async- detect string context on a getBody() call and return the entire body, maybe wrapped in a promise, instead?

Not sure how reasonable this would be. The basic idea is that buffering a streaming body is relatively easy, while streaming a buffered body is actually impossible. As such, my view would be to stick with streaming by default and provide an easy to use API to leverage buffering.

I wouldn't expect sync return values at all from this package but- especially for the client- it might be handy to return an entire body instead of in parts.

And with sync values I more meant a complete ready to use PSR-7 request, as multiple people expressed their concerns about in #28 .

I understand that a number of people are concerned about a lack of interoperability, but shouldn't this point be moot if we provide a buffering API that is specifically aimed at achieving interoperability?

As an alternative, what do you think about basing this on top of middlewares (via #179) an provding some kind of API sugar like this:

$server = new Server(Http\Buffered(function (ServerRequestinterface $request) {

});

@WyriHaximus
Copy link
Member Author

Thus renaming it to StreamingServer, making the Server workings more obvious, is IMHO the way to go.

Wouldn't hurt, but on the other side everybody using react should meanwhile be aware that he's in async wonderland.

I concur, I'm not opposed, but how much would be gained by this BC break?

Mainly a clearer distinction between the two servers if we go for two. But there is no need for a change if we go the route you proposed at the end of your comment.

As an alternative, what do you think about basing this on top of middlewares (via #179) an provding some kind of API sugar like this

That would work, doing the same in @php-api-clients where buffering is just a middleware in the HTTP client 👍 .

I wouldn't expect sync return values at all from this package but- especially for the client- it might be handy to return an entire body instead of in parts.

And with sync values I more meant a complete ready to use PSR-7 request, as multiple people expressed their concerns about in #28 .

I understand that a number of people are concerned about a lack of interoperability, but shouldn't this point be moot if we provide a buffering API that is specifically aimed at achieving interoperability?

IMHO the interoperability is a welcome side affect of something that was planned before PSR-7 support came into the code base.

@clue
Copy link
Member

clue commented Sep 8, 2017

@WyriHaximus Do you feel this ticket is still relevant now that #179 is in via #215? It's my understanding that the current Server will stay as-is and we will provide a buffering implementation on top? :shipit:

@WyriHaximus
Copy link
Member Author

@clue this issue has become irrelevant through #215 and #216

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

No branches or pull requests

3 participants