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

Add option for custom fetch implementation #273

Merged
merged 17 commits into from
Jul 20, 2020
Merged

Add option for custom fetch implementation #273

merged 17 commits into from
Jul 20, 2020

Conversation

jonasgeiler
Copy link
Contributor

@jonasgeiler jonasgeiler commented Jul 17, 2020

As requested in #269, this PR adds the ability to pass a fetch option when creating a ky instance.
This allows to use custom fetch implementations like isomorphic-unfetch or when using SSR in some frameworks (Sapper in my case).

TODO

  • Implementation
  • Documentation
  • Tests

Fixes #269

@jonasgeiler
Copy link
Contributor Author

Definitely need some help with the tests as I don't have any experience with ava or whatever you're using here

@sindresorhus
Copy link
Owner

Definitely need some help with the tests as I don't have any experience with ava or whatever you're using here

Give it a try and we'll be happy to help if you get stuck.

https://github.com/avajs/ava/blob/master/docs/01-writing-tests.md

@sholladay
Copy link
Collaborator

The tests for this feature should go in here:
https://github.com/sindresorhus/ky/blob/master/test/fetch.js

You can run the tests in that file with a command like this:

npx ava test/fetch.js

Or to run all of the tests:

npx ava

@jonasgeiler
Copy link
Contributor Author

jonasgeiler commented Jul 18, 2020

Just took the other test in test/fetch.js and modified it to use the option instead of the global for the custom fetch function - hope this is enough!
Let me know if you need anything else

@jonasgeiler
Copy link
Contributor Author

@sholladay @sindresorhus Is this ready to merge?

test/fetch.js Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Ability to provide custom fetch Add the ability to provide custom fetch implementation Jul 19, 2020
test/fetch.js Outdated Show resolved Hide resolved
test/fetch.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@jonasgeiler jonasgeiler requested a review from sholladay July 20, 2020 08:01
@jonasgeiler jonasgeiler requested a review from sindresorhus July 20, 2020 09:08
Copy link
Collaborator

@sholladay sholladay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. :)

@jonasgeiler
Copy link
Contributor Author

@sholladay yup! Ready to merge on my side 🚀

@sholladay sholladay changed the title Add the ability to provide custom fetch implementation Add option for custom fetch implementation Jul 20, 2020
@jonasgeiler
Copy link
Contributor Author

I think the travis build is broken @sholladay

@sholladay
Copy link
Collaborator

Hmm. The logs on Travis indicate that the build completed successfully, but for some reason GitHub shows the build as still running.

@sholladay sholladay merged commit ccda3b9 into sindresorhus:master Jul 20, 2020
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

Successfully merging this pull request may close these issues.

Can external "fetch dependency be injected to ky"
3 participants