-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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 a fetch
option to allow wrappers
#229
Conversation
self
option to allow wrappersfetch
option to allow wrappers
From what I see this saves you from duplicating code, basically allow you to hook into the redirection process which is where automate cookie handling is desired, true? @TimothyGu I think this makes sense, just wondering if |
Exactly. 👍 |
|
I can rename it to |
@valeriangalliat I think @TimothyGu what do you think, I do want this to happen, because it's one of the most discussed pain point of |
This way a wrapper library like fetch-cookie [1] could transparently leverage node-fetch way of handling redirects without having to force `{ redirect: 'manual' }` and re-implement the logic. [1]: https://github.com/valeriangalliat/fetch-cookie Instead, a wrapper could just pass the `resolver` option and point it to the wrapped function. And this would also allow wrappers to be combined as long as they also support being given the `resolver` option, e.g.: function fetchCookie(url, opts) { return fetch(url, Object.assign({ resolver: fetchCookie }, opts)); }
@bitinn I added comment, test and documentation for the |
For now this PR looks fine to me. On the long term, however, I envision a mechanism to have a "store" of Web interfaces including The approach I have on my mind is to exports an additional So to implement a fuller-featured // isomorphic-fetch-node.js
import { URL, URLSearchParams } from 'url';
import { Blob } from 'file-api';
import { FormData } from 'form-data';
import { createFetch } from 'node-fetch';
import { CookieJar } from 'tough-cookie';
export createFetch({
Blob,
CookieJar,
FormData,
URL,
URLSearchParams
}); // isomorphic-fetch-browser.js
import 'whatwg-fetch';
export { fetch, Headers, Request, Response }; And for traditional uses of the API can be extended: import { Blob, readBlob } from 'file-api';
import createFetchCookie from 'fetch-cookie';
import originalFetch from 'node-fetch';
const fetch = createFetchCookie(originalFetch);
(async () => {
const res = await fetch(url, {
method: 'POST',
body: 'abc',
interfaces: {
Blob,
fetch // this line might not even be necessary
}
});
const blob = await res.blob();
const buf = await readBlob(blob);
// Buffer.isBuffer(buf);
})(); |
Codecov Report
@@ Coverage Diff @@
## master #229 +/- ##
==========================================
- Coverage 100% 98.97% -1.03%
==========================================
Files 6 6
Lines 383 392 +9
Branches 117 118 +1
==========================================
+ Hits 383 388 +5
- Misses 0 2 +2
- Partials 0 2 +2
Continue to review full report at Codecov.
|
I agree this would be the superior approach, but significant work needs to be done to support this modularize approach. Anyone up to the task? (Also color me a bit wary of adding features most don't use) |
Closing, as we need a bit more time to figure out the best way to tackle this issue. |
This way a wrapper library like fetch-cookie could transparently leverage node-fetch way of handling redirects without having to force
{ redirect: 'manual' }
and re-implement the logic.Instead, a wrapper could just pass the
fetch
option and point it to the wrapped function. And this would also allow wrappers to be combined as long as they also support being given thefetch
option, e.g.: