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

keeping arbitrary options on fetch #431

Merged
merged 2 commits into from Jun 5, 2019
Merged

Conversation

@nynkelys
Copy link
Contributor

nynkelys commented May 28, 2019

closes #430

@wheresrhys

This comment has been minimized.

Copy link
Owner

wheresrhys commented May 28, 2019

I was a little nervous of this. Cloudflare are calling fetch with a Request instance and an options object, which I've never seen done before (if using a Request instance, you'd normally pass the options into the Request constructor), so I thought it might be a non-standard extension of the API. But have tried this scenario in Chrome, and it seems to work, blending the new options with those previously existing in the Request
image

... so I guess it's fine.

My only problem with this PR in that case is that the tests you've written are testing the wrong code path; your tests test passing a url with options, not a Request with options, and the library already supported (accidentally) arbitrary options in the former case. Could you add a new test where you call fetch using a Request?

Exciting it's been used to test cloudflare workers. Yet another environment where fetch is being used

@wheresrhys

This comment has been minimized.

Copy link
Owner

wheresrhys commented Jun 5, 2019

Great thanks. Will release it shortly as a patch

@wheresrhys wheresrhys merged commit 28744cd into wheresrhys:master Jun 5, 2019
2 checks passed
2 checks passed
codeclimate All good!
Details
security/snyk - package.json (wheresrhys) No manifest changes detected
@nynkelys

This comment has been minimized.

Copy link
Contributor Author

nynkelys commented Jun 6, 2019

Thanks Rhys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.