Replace xhr with native fetch #290
Conversation
pablopalacios
force-pushed
the
use-fetch
branch
6 times, most recently
from
26e722c
to
de895c3
Compare
|
@redonkulus I hope you haven't started reviewing it, I just realized that I sent the dirty branch. |
pablopalacios
force-pushed
the
use-fetch
branch
from
de895c3
to
e7d8624
Compare
|
Now it should be easier to review. |
|
Thanks for the update, looks like some conflicts after I merged your previous PR. |
pablopalacios
force-pushed
the
use-fetch
branch
from
e7d8624
to
ff7963c
Compare
|
@redonkulus done ;) |
|
Sorry merged another of your PRs and created conflicts. One more rebase please. |
pablopalacios
force-pushed
the
use-fetch
branch
from
ff7963c
to
ee5d293
Compare
|
@redonkulus no prob, done again :) |
README.md
Outdated
|
|
||
| For some applications, there may be a situation where you need to process the service params passed in XHR request before params are sent to the actual service. Typically, you would process these params in the service itself. However, if you want to perform processing across many services (i.e. sanitization for security), then you can use the `paramsProcessor` option. | ||
| For some applications, there may be a situation where you need to | ||
| process the service params passed in the request before they are sent |
There was a problem hiding this comment.
Does your editor auto break the lines at a certain character count or is prettier doing this?
There was a problem hiding this comment.
My editor. Should I undo it?
There was a problem hiding this comment.
Ya I like having it on one line, makes it easier to edit later without having to keep changing line breaks.
| Request: nodeFetch.Request, | ||
| }); | ||
|
|
||
| if (!global.fetch) { |
There was a problem hiding this comment.
which envs are we testing in that do not support fetch?
There was a problem hiding this comment.
nodejs itself does not support fetch (all versions). So, in order to run the unit-tests we need to polyfill fetch (we were also mocking xhr before, either with mockery on http tests, either with sinon fake xhr on fetchr.client tests).
pablopalacios
force-pushed
the
use-fetch
branch
from
ee5d293
to
9629043
Compare
There was a problem hiding this comment.
Code changes look good. @pablopalacios @redonkulus are we planning to bump major to 1.0, since apps will need to add fetch polyfill after this change?
|
@lingyan thanks for the review. This is a good question actually. We did some breaking changes when we moved from 0.5 to 0.6, so I would be fine moving to 0.7 now. If we go to 1.0, then changing the options API (#275) would require a bump to 2.0. But I would like to avoid many major bumps in a sequence. To avoid that I would close this PR and create a new one replacing |
|
I would not go to 1.x, saving that for the big api refactor. We just released 0.6 so I’m not too concerned about getting this out in a patch version. I doubt we have many users on this version anyways. |
|
Theoretically we should bump to 0.7 or 1.0. But if we know there are not other users using 0.6, I'm fine with bumping patch. |
|
I would go with 0.7. |
|
Ok will merge and release 0.7 to be safe |
|
0.7 released https://github.com/yahoo/fetchr/releases/tag/v0.7.0 Thanks for all the hard work you are doing! |
|
It's so nice how the package looks now on bundlephobia: |
|
wow ya its definitely trending downward |
What is done
What I didn't do (but I will do in future PRs)
Maybe the github diff is not that helpful here. But as I said, I tried to not change the code much. Mainly, the
iofunction fromhttp.clientmodule had to be updated. The place that I think is super critical to review is the timeout implementation (so I recommend you to triple check that).I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.