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

RFI: Deprecation of send() #178

Closed
shaunbramley opened this issue Jul 13, 2020 · 2 comments
Closed

RFI: Deprecation of send() #178

shaunbramley opened this issue Jul 13, 2020 · 2 comments

Comments

@shaunbramley
Copy link

Changeset as listed in #170 highlights that Browser::send() has been deprecated. #170 also makes reference to #56, #154, #162, #167, and #168; all of which revolve around API updates and test suite improvements.

As per a DM with @clue I am submitting this "issue" as an RFI for technical reasons behind deprecation of send().

It feels odd that a "PSR-7 HTTP client" (shamelessly copied from README.md) does not (or rather will not) have a publicly available method that sends a previously created PSR-7 request.

The transactional methods (get, put, head, post, etc, etc) all proxy through requestMayBeStreaming() which generates a PSR-7 request which in turn is used in Transaction::send().

Is the intent to shift away from PSR-7 as the browser moves forward (under react/http)?

send() made it easier to move from Guzzle to a React based application. My most common use case is that the browser is wrapped in a middleware type architecture where the request type is not necessarily known (or cared about) and specific actions are added onto the promise chain based on specific criteria.

$handler = function (RequestInterface $request, array $options) use ($browser) : PromiseInterface {
    // configure browser if required based on $options.

    // send the request
    return $browser->send($request);
};

$middleware = function (RequestInterface $request, array $options) use ($handler) : PromiseInterface {
    $promise = $handler($request, $options);
    if ($request meets some condition) {
        $promise->then(function(ResponseInterface $response) {
            // do some things;
         });
    }
    return $promise;
};
@clue
Copy link
Owner

clue commented Jul 13, 2020

@shaunbramley Thanks for bringing this up, I think it's an excellent question!

The send() method has been deprecated via #170, but it continues to work as-is. You can keep using it until you update to react/http as per #177.

The main motivation for this is that the previous options have been replaced with dedicated methods to configure the Browser. In particular, the new request() and requestStreaming() methods are believed to provide a much cleaner interface to sending arbitrary requests.

The send() method requires an explicit RequestInterface object, which in most cases means that consumers also need an explicit dependency on a PSR-7 implementation (which isn't exposed by this project, i.e. there's no public Request class). It's too easy to have state attached to this request object which should not leak through to the network layer. For instance, most PSR-7 implementations default to creating requests with the HTTP/1.1 protocol version. With #162, this decision is left up to the Browser, i.e. if we start supporting HTTP/2 in the future (reactphp/http#99), we would have to stick to older protocol versions in many cases or disregard the caller arguments. Likewise, a PSR-7 implementation may default to HTTP/2 right now and we would not be able to fulfill this request due to lack of protocol support. Additionally, most PSR-7 implementations will not support passing a streaming request body using the ReadableStreamInterface, which is easy to support in the request()/requestStreaming() methods.

Is the intent to shift away from PSR-7 as the browser moves forward (under react/http)?

Not at all. I think PSR-7 message abstractions are an excellent choice for this library (and react/http).

I think we can still discuss having (i.e. re-introducing) a send() method if the above points are taken care of. Additionally, we may also have to implement another way to send a request that receives a streaming response à la requestStreaming(). A new sendStreaming() could be the answer, but I feel this API name is misleading as it is more about receiving a streaming response body rather than sending a streaming request body. Sending a streaming request body is already supported by using a ReadableStreamInterface for either of the request methods.

At the moment, I think the cons outweigh the pros and using this below logic seems like a good compromise:

// old
$browser->send($request);

// new
$browser->request(
    $request->getMethod(),
    (string) $request->getUri(),
    $request->getHeaders(),
    (string) $request->getBody()
);

It's not perfect, but it gets the job done. I'm happy to hear your thoughts on this 👍

@clue
Copy link
Owner

clue commented Jul 23, 2020

I believe this has been answered, so I'm closing this for now. Please come back with more details if this questions persists and we can reopen this 👍

@clue clue closed this as completed Jul 23, 2020
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

2 participants