refactor(useFetch): change internal logic #549

userquin · 2021-05-30T10:46:23Z

closes #542
closes #539
closes #553
closes #526

userquin · 2021-05-30T11:32:07Z

Dont merge yet, I Will change this PR to correct content-type and we need also handle form data in the assignment if the payload type is not provided.

feat: added `formEncoded` allowing submission of forms feat: added `application/x-www-form-urlencoded` handling via URLSearchParams

userquin · 2021-05-30T12:20:55Z

the headers should be handled before the fetch call, since the user can provide them on context. If multipart/form is requested and the user include the header, the server will fail to extract submitted files.

chore: adjust tests to use Headers instance

userquin · 2021-05-30T13:57:43Z

I need to do one more thing, encode the data when using formEncoded when payload is not URLSEarchParams, so don't merge yet, I'm working on it rigth now.

userquin · 2021-05-30T14:16:54Z

I need to add some tests for application/x-www-form-urlencoded and multipart/form-data payload types.

userquin · 2021-05-30T14:40:14Z

@wheatjs it is confusing the logic inside execute method: since we can override the body from context options, and before calling fetch we are using context.options after defaultOptions when merging to create the RequestInit object, the logic handling the payload and the headers can be moved before the Promise and we can also use context.options.body if provided instead the config one.

I think we can change the behavior and so we can use the context.options to reuse the execute method. I will wait your feedback before changing anything.

userquin · 2021-05-30T15:30:05Z

@wheatjs I will change it so that you can compare with the original, looking at changes you will see what I mean.

userquin · 2021-05-30T15:53:01Z

@wheatjs I cannot figure out how to include jest tests for multipart/form-data and for application/x-www-form-urlencoded, since we are mocking the request.

…-data

userquin · 2021-05-30T16:43:34Z

@wheatjs the only way I have found is to include it on the demo (I will also add application/x-www-form-urlencoded in the page):

once executed (can be executed multiple times, with blob content changing between request)

userquin · 2021-05-30T17:00:16Z

Also application/x-www-form-urlencoded included:

once executed (can be executed multiple times, with form content changing between request)

…ded` to the demo page

userquin · 2021-05-30T17:48:37Z

@wheat

With this PR useFetch will work as expected, I'll try to fix the change of the content type once execute is called first time.

I don't know why don't allow to change the config object once initialized, I think it can be modified just not allowing to change it while fetch is in progress.

Why is setMethod returning undefined if initialized is true? Is this intentional or something that was included time ago and now it is not necessary?

wheatjs · 2021-06-02T17:21:59Z

Why is setMethod returning undefined if initialized is true? Is this intentional or something that was included time ago and now it is not necessary?

This is on purpose as we don't want to allow users to change the method after they've already set it. I think this is probably okay to merge though @antfu. There are a few other changes that need to be made to useFetch before v5.0 official release so I'd like to get this finished so I can work on the others without to many merge conflicts.

…part on code

userquin · 2021-06-03T16:32:28Z

I think the PR is done, waiting for reviews, tmr I'm taking a vacation until next Monday / Tuesday ... but I'll do it with an eye here.

…d` and `formData` chore: remove extra commas and change tabs size from the examples

userquin · 2021-06-03T20:19:27Z

One last thing I must include: some tests for encoding. I cannot figure out how to test formEncoded and formData with jest, since we need to mock request and response and there is no real server behind.
See you next week...

…formEncoded` and `formData` docs: rephrasing some entries on `formEncoded` and `formData`

wheatjs · 2021-06-04T00:11:41Z

I feel like this PR is doing to much and it is getting hard to review. Perhaps you can refocus this PR on the original problem is was solving and we can address the other fetch issues on another branch

userquin · 2021-06-04T00:30:41Z

I feel like this PR is doing to much and it is getting hard to review. Perhaps you can refocus this PR on the original problem is was solving and we can address the other fetch issues on another branch

Can you create a branch and merge this pr on it? Maybe this will help to review it.

benlesh · 2021-06-08T23:25:59Z

packages/core/useFetch/index.md

+      error: false,
+      errorMessage: undefined
+    }
+    switch(response.status) {


Seems like this should be:

if (response.ok) { context.data = await response.blob(); } else { // handle exceptional cases here. }

benlesh

Sorry for the drive-by review. I saw this PR in here while I was submitting one of my own, and I've recently done a lot of work in this area (for RxJS).

This PR looks like it was a lot of very well thought out work and consideration, but I would advise against merging it in its current state for a few reasons:

It adds a lot more weight to what should otherwise be a pretty light-weight helper.
The handling of setting content-type is weighty and incorrect.
The area that deals with URLSearchParams will add additional JSON parsing burden to any server consuming what is (in the majority case) supposed to be simple value types. And it's a lot of unnecessary work.
Most of the ergonomics gains that could be made from this PR seem like they may result in unintentional pain and a lot of extra weight to cover what, in my opinion, would be edge cases that developers could easily handle with their own code.

Again, this is just my general review that was completely unsolicited, and I really respect the author for putting in the effort and sticking to the debate in this PR.

I think maybe a much leaner, smaller, incremental step, with obvious value, might be attainable by refactoring this PR.

benlesh · 2021-06-08T23:37:01Z

packages/core/useFetch/index.ts

@@ -168,6 +185,46 @@ function isFetchOptions(obj: object): obj is UseFetchOptions {
  return containsProp(obj, 'immediate', 'refetch', 'beforeFetch', 'afterFetch')
 }

+async function toURLSearchParams(body: any): Promise<URLSearchParams> {


I'm not sure I understand the purpose of this function. The goal seems to be to deal with the fact that the user may have accidentally passed a search param that is an object, rather than a primitive value type.

I'm not sure why it's asynchronous. I don't see any async work being done.

It doesn't seem like a good idea to JSON encode everything, because then whatever is on the other end of the network will have to JSON decode each value. In other words, it will get a bunch of key/value pairs, and then be forced to JSON.parse (or the server language equivalent) for every single value in there, even if they're all strings.

I strongly think the onus of doing this work (and deciding if it's even necessary) should be on the developer making the fetch, and we should throw this logic out and let them handle it. They know their server (presumably). And this is covering a use case that generally doesn't exist in the 99% case. Most of the time, if someone is sending search params, they're value-type primitives, not objects. This is penalizing the 99% use case to accommodate a one-off scenario.

benlesh · 2021-06-08T23:44:44Z

packages/core/useFetch/index.ts

-    if (config.payload) {
+    let payload = context.options?.body || config.payload
+
+    if (payload) {


Most of the work below here does not need to be done. fetch and it's related types, like Request, will automatically figure out the proper Content-Type header.

It's worth noting that this is even the case with XHR. I recently wrote some code related to this for XHR, which handles this that is much more concise. Hopefully that helps.

I strongly advise that we don't do things like check a payloadType configuration or the like, because it will lead to user confusion and odd behaviors.

We need to avoid to include a breaking change.
It is a pain payloadType option, we must deal with it.
The content type must be addressed since can be wrong, for example, adding it when using multipart, you can see the pictures included in this pr.

From a user perspective, it Will be more useful to provide the payload as a raw object rather than using fetch ones,.

I'm waiting to be included in a separated branch.

wheatjs · 2021-07-16T19:12:09Z

Going to close this out as we plan on introducing useKy instead for advanced fetch usage.

fix: wrong payloadType assigment

fb7aff4

userquin mentioned this pull request May 30, 2021

useFetch: multipart form data header must be removed instead added #542

Closed

userquin changed the title ~~fix(useFetch): wrong payloadType assigment on setMethod~~ fix(useFetch): wrong payloadType assignment on setMethod May 30, 2021

userquin mentioned this pull request May 30, 2021

useFetch and formData doesn't work #539

Closed

fix: correct request content-type header

720db80

feat: added `formEncoded` allowing submission of forms feat: added `application/x-www-form-urlencoded` handling via URLSearchParams

userquin changed the title ~~fix(useFetch): wrong payloadType assignment on setMethod~~ fix(useFetch): correct content-type handling May 30, 2021

userquin added 3 commits May 30, 2021 14:48

fix: handle request content-type header before calling fetch

a4b0385

fix: adjust logic to remove headers from context

2edb23f

chore: adjust tests to use Headers instance

chore: clean up and add some comments

a70523b

chore: encode object when is not URLSearchParams nor a string

f80e708

chore: encode using async tasks

f939d9d

chore: reorder execute method logic

b7f94ac

chore: minor changes to remove content-type header for multipart/form…

31857bc

…-data

feat: added multipart/form-data and `application/x-www-form-urlenco…

152a653

…ded` to the demo page

userquin added 5 commits June 2, 2021 20:06

chore: allow call setMethod if payloadType not modified

f6ec788

chore: allow call setMethod if payloadType not modified

bfc78ca

chore: allow change the payload not the method

9148530

fix: move configuring method inside anom fun

f09f227

fix: max file upload input size 10KB

7025a4d

isgj mentioned this pull request Jun 3, 2021

useFetch Have access to the response body when the status is not ok #553

Closed

userquin added 10 commits June 3, 2021 11:20

chore: modify form url encoding logic

99c356b

chore: fix typo

37221e4

chore: fix typo on max file size validity message

bd82304

fix: avoid double encoding on json payload

2ee56d8

chore: handle raw object also for formData

5787734

feat: custom response handler

a9adad1

chore: remove default values destructuring custom response call

893018c

chore: adjust custom response jsdoc

718ae14

chore: modify toFormData to deal with nested objects

69cb1e4

chore: remove nested if statements on toFormData

ad81a58

userquin changed the title ~~fix(useFetch): correct content-type handling~~ refactor(useFetch): change internal logic Jun 3, 2021

userquin added 5 commits June 3, 2021 16:27

chore: remove method from beforeFetch context to avoid changing it

9f7a825

chore: document why we need to remove content-type header for multi…

d178a7a

…part on code

chore: add docs to index.md + some wording on events entry

8d1f05f

chore: second round for docs

b5986bc

docs: add custom response handler on type declarations

ba8c165

userquin requested a review from wheatjs June 3, 2021 16:29

userquin added 2 commits June 3, 2021 21:06

docs: fix missing breaks on advanced usage example

ef2b3e4

docs: include nested objects handling for raw objects for `formEncode…

b8f3e66

…d` and `formData` chore: remove extra commas and change tabs size from the examples

docs: reordering of comments regarding the use of nested objects on `…

147c5e8

…formEncoded` and `formData` docs: rephrasing some entries on `formEncoded` and `formData`

benlesh reviewed Jun 8, 2021

View reviewed changes

benlesh suggested changes Jun 8, 2021

View reviewed changes

isgj mentioned this pull request Jun 10, 2021

feat(useFetch): set content type only if it is json #573

Merged

wheatjs closed this Jul 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(useFetch): change internal logic #549

refactor(useFetch): change internal logic #549

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021

userquin commented May 30, 2021

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021

userquin commented May 30, 2021

wheatjs commented Jun 2, 2021 •

edited

userquin commented Jun 3, 2021

userquin commented Jun 3, 2021

wheatjs commented Jun 4, 2021

userquin commented Jun 4, 2021

benlesh Jun 8, 2021

benlesh left a comment •

edited

benlesh Jun 8, 2021

benlesh Jun 8, 2021

userquin Jun 9, 2021

wheatjs commented Jul 16, 2021

refactor(useFetch): change internal logic #549

refactor(useFetch): change internal logic #549

Conversation

userquin commented May 30, 2021 • edited

userquin commented May 30, 2021

userquin commented May 30, 2021 • edited

userquin commented May 30, 2021 • edited

userquin commented May 30, 2021

userquin commented May 30, 2021 • edited

userquin commented May 30, 2021

userquin commented May 30, 2021

userquin commented May 30, 2021 • edited

userquin commented May 30, 2021

userquin commented May 30, 2021

wheatjs commented Jun 2, 2021 • edited

userquin commented Jun 3, 2021

userquin commented Jun 3, 2021

wheatjs commented Jun 4, 2021

userquin commented Jun 4, 2021

benlesh Jun 8, 2021

Choose a reason for hiding this comment

benlesh left a comment • edited

Choose a reason for hiding this comment

benlesh Jun 8, 2021

Choose a reason for hiding this comment

benlesh Jun 8, 2021

Choose a reason for hiding this comment

userquin Jun 9, 2021

Choose a reason for hiding this comment

wheatjs commented Jul 16, 2021

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021 •

edited

userquin commented May 30, 2021 •

edited

wheatjs commented Jun 2, 2021 •

edited

benlesh left a comment •

edited