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

API suggestion: allow RequestBuilder to specify URI, method, and content without sending the request #29

Closed
bryanrsmith opened this issue Mar 21, 2015 · 3 comments

Comments

@bryanrsmith
Copy link
Contributor

I think RequestBuilder's API should be optimized for building requests, and not necessarily sending them. Currently the methods used to specify the URI, HTTP method, and request body/content also send the request, making it impossible to build a request without sending it. It should be possible to use this class to build a complete request and still maintain control over when it's sent.

I also think the semantics of these methods are a little unclear. Consider the code,

httpClient.createRequest()
    .post(uri, content)

Asking a request to post something doesn't make sense because "post" is a property or attribute of a request. It's something a client does, not something a request does. It makes more sense to tell the request that it uses "post", and then ask for the request to be sent.

I propose the following changes to the RequestBuilder class:

  • Replace .get(), .post(), etc. methods with .with- style methods that specify the HTTP method with no side effect.
  • Add .withUri() and .withContent().
  • Add a .getRequest() method that returns a complete HttpRequestMessage.
  • Add a .send() convenience method to let the API stay fluent and chain-able.

We could also consider allowing the URI and HTTP method to be passed as parameters to HttpClient.createRequest().

Current API:

client.createRequest()
    .withParams({ foo: 'bar' })
    .get('/cool/stuff');

client.createRequest()
    .withParams({ foo: 'bar' })
    .post('/cool/stuff', { some: 'content' });

Proposed API:

client.createRequest('/cool/stuff') // or `.withUri('/cool/stuff')`
    .withGetMethod() // it seems reasonable to make GET the default, and allow this to be omitted
    .withParams({ foo: 'bar' })
    .send();

client.createRequest('/cool/stuff')
    .withPostMethod() // or .withMethod('POST')
    .withContent({ some: 'content' })
    .send();
@EisenbergEffect
Copy link
Contributor

What about asGet() and asPost() or withMethod.get() and withMethod.post()?

@bryanrsmith
Copy link
Contributor Author

I like asGet() and asPost(). That reads nicely. The main goal of my suggestion was to separate the building from the sending. I don't feel particularly strongly about the method names. I will admit to a slight aversion to the (ab?)use of getters in fluent APIs. It's just personal preference, though :-)

bryanrsmith added a commit to bryanrsmith/http-client that referenced this issue Mar 22, 2015
… without sending the message

This change reworks the `RequestBuilder` API so that URI, HTTP method, and content can be specified without forcing the request to be sent.

This is a breaking API change to `RequestBuilder`. To update code, replaces uses of `.get(uri)`, `.post(uri, content)`, etc. with `.asGet().withUri(uri).send()`, `.asPost().withUri(uri).withContent(content).send()`.

Fixes aurelia#29
@bryanrsmith
Copy link
Contributor Author

I ended up not adding a .getRequest() method because the helpers all create transformers that aren't applied until the request is sent by the client, so the request created by RequestBuilder has basically no info on it.

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

No branches or pull requests

2 participants