amphp / http-client-psr7

PSR-7 adapter for amphp/http-client.
MIT License
10 stars 3 forks source link

Should PsrAdapter be splitted or not? #2

Open remorhaz opened 4 years ago

remorhaz commented 4 years ago

As for now, PsrAdapter class provides full set of methods to convert both Amp and PSR-7 requests and responses in both directions. There is an opinion that it could be split apart.

I think that most frequent use cases will consist of the following (maybe partially):

But, on the other hand, there are cases where "alien" code just alters messages and returns them back (adds headers, for example); in this case, we recieve the same class of object back, like this:

This case requires toPsrRequest() and fromPsrRequest() methods, so I decided that splitting the adapter for the first group of cases can harm users that implement second group of cases and vice versa.

I've looked through @trowski 's implementation of server message bridge; it just implements MessageConverter interface that declares methods convertRequest() (that works like my toPsrRequest()) and convertResponse() (that works like my fromPsrResponse()). Thus, it allows only first use case.

@kelunik, what do you think about it?

kelunik commented 4 years ago

Depending on the use case you need every combination, yes. Main reason for splitting would be, because not every case needs the factories.

I've added https://github.com/amphp/http-client-psr7/commit/aed833358dbcc86010bf941375bc6ce7eab7cfd1, which should simplify the usage.

kelunik commented 4 years ago

What should we do with https://github.com/amphp/http-client-psr7/blob/aed833358dbcc86010bf941375bc6ce7eab7cfd1/src/Internal/PsrMessageStream.php? Seems to be unused currently.

remorhaz commented 4 years ago

Seems to be unused currently.

It was designed as an honest bridge between Amp's and PSR's way of sequential reading the body, but it was blocked by amphp/amp#311 and amphp/amp#313; so now it looks like an overkill.

I can either remove it (and rewrite it back when it becomes possible) or use it instead of PsrAdapter::copyToPsr() private method (this option may require less work to do after wait() issues get closed). I'm totally okay with both options; which one do you prefer?

kelunik commented 4 years ago

I prefer not using it currently, but we could also allow to configure that behavior. What do you think about that?

remorhaz commented 4 years ago

I think there's nothing of value to configure yet. I'll remove it for now and will get back to it when nested loops run okay.