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

FIX: defined headers should be honored *over* injected headers #17

Closed
kwhitley opened this issue Oct 9, 2022 · 5 comments · Fixed by #18
Closed

FIX: defined headers should be honored *over* injected headers #17

kwhitley opened this issue Oct 9, 2022 · 5 comments · Fixed by #18
Labels
bug Something isn't working

Comments

@kwhitley
Copy link
Owner

kwhitley commented Oct 9, 2022

Expected Behavior

  • If I set a content-type header, I should expect it to be honored, no matter what
  • If I set a body (manually) in the options, I should expect it to be honored, no matter what

Actual Behavior

  • If I set a content-type header, application/json will be sent anyway
  • If I set a body (manually) in the options, the payload is sent instead

Use Case

I am attempting to send blob data through a fetcher instance. I can do this with native fetch, but not itty-fetcher:

const blob = new Blob(["This is some important text"], { type: "text/plain" });

fetch('https://ity.sh/create', { method: 'POST', body: blob })
@kwhitley kwhitley added the bug Something isn't working label Oct 9, 2022
@danawoodman
Copy link
Collaborator

Are you sure you can't override the content-type? I just added a test for it and passing { headers: { "Content-Type": "text/plain" } } and it works.

@danawoodman
Copy link
Collaborator

@kwhitley I've fixed the body issue in #18 as well as moved us to jsdom test env. I also added a test to verify the content-type header changing does work as intended

@kwhitley
Copy link
Owner Author

Thanks @danawoodman! Real quick on this, what would be the proposed way to send blob content? Currently, we attempt to JSON.stringify everything but FormData.

Currently in native fetch, you can do something like

fetch('/endpoint', {
  method: 'POST', 
  body: blob, // content-type is sent without explicitly defining
})

without having to explicitly define or override content-type... this allows you to easily pass files without extra steps. To support that in fetcher, we'd have to manually override the pre-embedded application/json header currently.

@kwhitley
Copy link
Owner Author

kwhitley commented Oct 11, 2022

I'm leaning towards these behaviors:

  • if payload is Blob, FormData, or other valid object type for passing straight to request.body, we pass it through to body unchanged, with no explicit content-type header
  • else, we assume JSON and stringify/embed header

Thoughts? I just don't want to be more limiting than native fetch through our assumptions, nor make folks jump through hoops to even replicate what they were able to do easily before.

Ideal interface

const body = new Blob(['something here'],  { type: 'text/plain' })

fetcher().post('/endpoint', body)
// would be equivalent to
fetch('/endpoint', { method: 'POST', body })

kwhitley added a commit that referenced this issue Oct 11, 2022
[#17] Allow manually sending "body" in request init
@danawoodman
Copy link
Collaborator

danawoodman commented Oct 11, 2022

@kwhitley you can send arbitrary body content now, see the tests here: https://github.com/kwhitley/itty-fetcher/pull/18/files#diff-2aa640fd9240ec144ac5019315d5631742a2b12d957395a0cc6ff4a0d9cf3f02R135. My change means you can manually pass in whatever body you want and we don't stringify it.

If you want to send a diff type, I don't see passing a header as a big deal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants