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

feat(useFetch): new function #330

Merged
merged 25 commits into from Feb 23, 2021
Merged

feat(useFetch): new function #330

merged 25 commits into from Feb 23, 2021

Conversation

wheatjs
Copy link
Member

@wheatjs wheatjs commented Feb 12, 2021

Here is the initial implementation of useFetch, as mentioned in #327.

General Feature Overview

  • Abort Requests if the browser supports it
  • Auto refetch requests if the url is a ref and the user has enabled autoRefetch
  • Reactive status, fetching state, finished stated, error, and canAbort
  • Ability to prevent the first initial fetch and manually call the fetch with the execute function

The signature for useFetch provides multiple overrides so it can be used in any of the following ways

useFetch(url: MaybeRef<string>)
useFetch(url: MaybeRef<string>, useFetchOptions: UseFetchOptions)
useFetch(url: MaybeRef<string>, options: RequestInit, useFetchOptions?: UseFetchOptions)

Let me know what you think and if you would like anything changed!

antfu and others added 3 commits February 9, 2021 23:32
BREAKING CHANGES:
- the `ws` option in the return of `useWebSocket` is not a ref instead of plain WebSocket instance
- `useWebSocket`'s `send` will now buffers data before the connection established instead of discarding sliently
- `useWebSocket`'s returned `state` is renamed to `status`
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
wheatjs and others added 4 commits February 12, 2021 11:26
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
@wheatjs
Copy link
Member Author

wheatjs commented Feb 12, 2021

Everything you mentioned should be fixed now

wheatjs and others added 3 commits February 12, 2021 11:45
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
packages/core/useFetch/index.ts Outdated Show resolved Hide resolved
if (contentType && contentType.includes('application/json'))
fetchResponse.json().then(json => data.value = json)
else
fetchResponse.text().then(text => data.value = text)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we make this usage simpler with some API?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, is there something out there that would allow us to simplify it? I'm not aware of anything, but if you know I'll update it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really. But maybe keep your implementation and make a useFetchJson wrapper with preconfigured header? Moreover, something like usePost and usePostJson?

Another approach might be similar to ky

useFetch(URL, {}).json()

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm doing more research into the fetch API they actually have more than just text() and json(), https://developer.mozilla.org/en-US/docs/Web/API/Body#methods

If we were to make a wrapper for all of these we would have useFetch, useFechJson, useFetchBlob, useFetchFormData, and useFetchArrayBuffer. But I feel like most of these would not be used very often and it would create quite a few methods. Perhaps another option would be to have an option in the config to set how the data is parsed. Something like this perhaps?

useFetch(url, {
  // Not sure what a good name for this would be
  // It could be defaulted toauto which would try to automatically detect the type from the response headers
  readAs: 'auto' //could also be json, formData, blob, arrayBuffer, or text
})

I do like the idea of wrappers for usePost and usePostJson. If we do that would we also want ones for form-data and multipart form data?

Comment on lines 140 to 145
if (isRef(url)) {
if (isRef(options.refetch))
watch([options.refetch, url], () => unref(options.refetch) && execute())
else if (options.refetch)
watch(url, () => execute())
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (isRef(url)) {
if (isRef(options.refetch))
watch([options.refetch, url], () => unref(options.refetch) && execute())
else if (options.refetch)
watch(url, () => execute())
}
watch(
()=>{
unref(url)
unref(options.refetch)
},
() => unref(options.refetch) && execute()
)

Sorry for the formatting

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem, I can fix it in a sec

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't actually seem to work. Can you watch an unref? I was under the impression it just gave you back the raw value if it was a ref

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you test it? It's not watching an unref but the "accessing". unref is just a unified way to accessing here, the return value doesn't matter. Should work I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it looks like it does work, but the watch needs to have deep: true set

Copy link
Member

@antfu antfu Feb 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Em? I don't think it's necessary... 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird, it only seems to work when I set deep: true. Not really sure why

Copy link
Member Author

@wheatjs wheatjs Feb 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just created a repo to demonstrate the issue. It is a bit of a contrived example, but it should show the issue. It is in vite so you should be able to just clone it and run it https://github.com/jacobclevenger/vue3-watch-bug/blob/master/src/App.vue

wheatjs and others added 3 commits February 12, 2021 11:52
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
@antfu
Copy link
Member

antfu commented Feb 14, 2021

@jacobclevenger got a few ideas of the api design, will give a try when I am back from vacation. Please wait for this a while, thanks.

@wheatjs
Copy link
Member Author

wheatjs commented Feb 14, 2021

@jacobclevenger got a few ideas of the api design, will give a try when I am back from vacation. Please wait for this a while, thanks.

Alright no problem, I look forward to it. Enjoy your vacation!

@antfu
Copy link
Member

antfu commented Feb 19, 2021

Made some changes to the API (haven't tested it out, and maybe we should have some unit tests), wondering what you think? Thanks!

Copy link
Member Author

@wheatjs wheatjs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API design looks good, I'll test out functionality and see about some tests

test('basic get request', async() => {
const { statusCode } = useFetch(urls.get)

/**
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would like some input on the best way to go about writing these tests in this situation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment starts on line 13

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can make the execute() async. And they can also use when

await when(isFinished)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, I'll make those changes and create some basic tests

@antfu antfu changed the title feat(useFetch): initial implementation of useFetch feat(useFetch): new function Feb 20, 2021
@wheatjs
Copy link
Member Author

wheatjs commented Feb 22, 2021

@antfu Added in some basic tests and fixed a few bugs with the useFetch function. Let me know what you think and if you'd like anymore tests 😄

@antfu antfu enabled auto-merge (squash) February 23, 2021 03:39
@antfu antfu merged commit 889f234 into vueuse:main Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants