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

Can external "fetch dependency be injected to ky" #269

Closed
guillaume-chervet opened this issue Jul 6, 2020 · 7 comments · Fixed by #273
Closed

Can external "fetch dependency be injected to ky" #269

guillaume-chervet opened this issue Jul 6, 2020 · 7 comments · Fixed by #273
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@guillaume-chervet
Copy link

guillaume-chervet commented Jul 6, 2020

I would like to inject an enhanced fetch to ky (fetch is a dependency to ky).

In order to use with https://github.com/AxaGuilDEv/react-oidc/tree/master/packages/context-fetch in a "functionnal way"

Is it possible to do something like :

const parsed = await ky(fetch).post('https://example.com', {json: {foo: true}}).json();

?

Regards,

Guillaume

@guillaume-chervet guillaume-chervet changed the title Does external "fetch" dependency can be injected to ki ? Does external "fetch" dependency can be injected to ky ? Jul 6, 2020
@guillaume-chervet guillaume-chervet changed the title Does external "fetch" dependency can be injected to ky ? Can external "fetch dependency be injected to ky" Jul 6, 2020
@sholladay
Copy link
Collaborator

Would it be okay if it were just a normal Ky option, as below?

const parsed = await ky.post('https://example.com', {fetch, json: {foo: true}}).json();
const myKy = ky.extend({fetch});
const parsed = await myKy.post('https://example.com', {json: {foo: true}}).json();

@guillaume-chervet
Copy link
Author

Yes, it is

@sindresorhus sindresorhus added enhancement New feature or request help wanted Extra attention is needed labels Jul 6, 2020
@jonasgeiler
Copy link
Contributor

Isn't this a duplicate of #56?

@sholladay
Copy link
Collaborator

@skayo

Yes, you're correct, it is. And the concerns that Sindre expressed there are still valid.

However, it's worth noting that since then:

  • Ky gained an additional co-maintainer (me :))
  • We have made it easier to override our globals, including fetch (e.g. f107e7b and PR Move globals to getters to allow them to be mocked #153)
  • We created ky-universal which could potentially benefit from this feature
  • Ky has stabilized a lot, we haven't tagged a 1.0 release yet but I've been happily using it in production for quite some time

It's definitely worth reconsidering this.

@jonasgeiler
Copy link
Contributor

jonasgeiler commented Jul 17, 2020

@sholladay
Yeah that is true.
Might even need this for a project myself.
Do you know how much work this would be?
Maybe I'll do a PR...

@sholladay
Copy link
Collaborator

sholladay commented Jul 17, 2020

It should be pretty easy. You would need to update this part of the code where we execute the fetch, so that it would be something like options.fetch() instead of globals.fetch(). And you would set the default value for fetch to globals.fetch at the top of the Ky constructor, where we set our other option defaults. That should be about it, other than tests and docs.

@jonasgeiler
Copy link
Contributor

@guillaume-chervet @sholladay
Check out #273!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants