Let superagent handle request body #57
Conversation
acontreras89
force-pushed
the
ac-query-params
branch
from
c96f0d8
to
6a2c0a0
Compare
|
I've pushed a new commit cleaning up the adapter (and tests) a bit. Let me know if you want me to roll it back. This commit also added an assertion missing in one of the tests, as well as support for HEAD requests (constant was defined but not handled in the |
src/adapters/superagent.js
Outdated
|
|
||
| if (body) { | ||
| request.send(body); | ||
| if (!(method in httpMethods)) { |
There was a problem hiding this comment.
Now that I think of it, this is completely wrong as it is checking against the names of the constants, rather than their values. It works because they're the same.
acontreras89
force-pushed
the
ac-query-params
branch
from
6a2c0a0
to
aeb82ef
Compare
There was a problem hiding this comment.
@acontreras89 Talking with the team we decided that this is a reasonable change to make. Thanks for putting in the work for the proof of concept.
I just have one request. Once that change gets in and the tests pass I'd like to merge this.
| @@ -1,30 +1,13 @@ | |||
| import superagent from 'superagent'; | |||
| import * as httpMethods from '../constants/http-methods'; | |||
|
|
|||
| export const createRequest = (url, method) => { | |||
There was a problem hiding this comment.
can you keep this function and the switch statement for explicitness?
in addition, we can move to making this function not exported with your change in tests/. that would be great
There was a problem hiding this comment.
Done 👍 let me know if you need anything else
|
Also – heads up. I plan to merge #60 soon which might cause some disruption. Sorry for any inconvenience. |
|
What about this
|
acontreras89
force-pushed
the
ac-query-params
branch
from
aeb82ef
to
f9b6893
Compare
|
Looks great. I'm ok with adding HEAD method support too. Thanks for contributing! |
See #55 for motivation and discussion.