Add ability to pass extra options on each request #12
Conversation
pgom
force-pushed
the
enhancement/request-options
branch
from
36be370 to
fe8ad94
Compare
| request(url, body, headers = {}, options = {}) { | ||
| const { method = 'get' } = options; | ||
| const requestOptions = { | ||
| ...options, |
There was a problem hiding this comment.
Are you destructuring the options object before the declaration of the other properties to prevent anyone from overriding those specific properties?
| .then(() => { | ||
| expect(sdk.client.request).toBeCalledWith( | ||
| 'https://api.uphold.com/v0/foo', | ||
| 'get', |
There was a problem hiding this comment.
Why did we removed the get assert?
There was a problem hiding this comment.
The request method is not an argument anymore. It is now provided by the options object.
There was a problem hiding this comment.
So, we need to create new tests to cover this scenario.
There was a problem hiding this comment.
Added on a different file.
pgom
force-pushed
the
enhancement/request-options
branch
2 times, most recently
from
7375b76 to
a08e949
Compare
src/browser/services/fetch-client.js
Outdated
| export default class FetchClient extends Client { | ||
| request(url, method = 'get', body, headers = {}) { | ||
| const options = { | ||
| request(url, body, headers = {}, options = {}) { |
There was a problem hiding this comment.
Why did you removed the method and moved it to the options? Shouldn't the option only be an extra parameter to override all the other parameters?
There was a problem hiding this comment.
The method is passed to sdk as an option and then we chose to separate it into a different variable. This way and to avoid increase the argument size on this method, I opt to remove it and start using the one on the options object
pgom
force-pushed
the
enhancement/request-options
branch
4 times, most recently
from
c4f313d to
dc423ac
Compare
| fetchMock.mock('foo', {}); | ||
|
|
||
| return client.request('foo', 'post', 'bar', { 'Biz-Baz': 'buz' }) | ||
| return client.request('foo', 'get', 'bar', { 'Biz-Baz': 'buz' }) |
There was a problem hiding this comment.
Do we need this change?
| undefined, | ||
| { authorization: 'foo' } | ||
| { authorization: 'foo' }, | ||
| { headers: { authorization: 'foo' } } |
There was a problem hiding this comment.
According to the test name, this shouldn't be added, right?
There was a problem hiding this comment.
The test name was wrong and confusing. Updated.
pgom
force-pushed
the
enhancement/request-options
branch
from
dc423ac to
a8a2b8a
Compare
|
@SandroMachado updated |
pgom
force-pushed
the
enhancement/request-options
branch
from
a8a2b8a to
b04e9ea
Compare
pgom
force-pushed
the
enhancement/request-options
branch
from
b04e9ea to
6c08470
Compare
3 participants
Description
This PR updates the request client making it to possible to pass additional options on each request
Related issues