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

Axios examples #145

Merged
merged 5 commits into from
Nov 27, 2019
Merged

Axios examples #145

merged 5 commits into from
Nov 27, 2019

Conversation

Svish
Copy link
Contributor

@Svish Svish commented Nov 21, 2019

I asked for some examples using axios in #143. Since the useRequest wrapper hook I made for my own project seems to work well, as far as my newb useSWR skills can gather, I decided to just copy the basic and basic-typescript examples, and change them to use this wrapper hook. 🙂

Hope I haven't missed anything crucial 🤔

Copy link
Contributor

@sergiodxa sergiodxa left a comment

Choose a reason for hiding this comment

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

examples/axios-typescript/package-lock.json should not be committed, maybe you could add it to the root .gitignore like yarn.lock?

@Svish
Copy link
Contributor Author

Svish commented Nov 21, 2019

@sergiodxa Good catch! Deleted, and ignored. 🙂👍

@shuding
Copy link
Member

shuding commented Nov 25, 2019

Although JSON.stringify could make it work, I don't think we should recommend it.

useRequest is a nice abstraction 👍

@Svish
Copy link
Contributor Author

Svish commented Nov 25, 2019

@quietshu Could make it work? What would make it not work? Something I could fix? 🤔

examples/axios-typescript/libs/useRequest.ts Outdated Show resolved Hide resolved
examples/axios/pages/[user]/[repo].js Outdated Show resolved Hide resolved
const { data: response, error, isValidating, revalidate } = useSWR<
AxiosResponse<Data>,
AxiosError<Error>
>(request && JSON.stringify(request), () => axios(request || {}), {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add a comment here and mention it in the README (and the documentation in the future).
JSON.stringify is a hack and not the recommended way because:

  1. it's unnecessary and slow (runs on every render)
  2. it's not stable

2 means it can produce different keys for the same request. e.g.:
JSON.stringify({ params: { a: 1, b: 2 } }) // -> {"params":{"a":1,"b":2}}
JSON.stringify({ params: { b: 2, a: 1 } }) // -> {"params":{"b":2,"a":1}}

I think this will be better:

useRequest(request, deps = [], config = {}) {
  useSWR(deps, () => axios(request || {}), ...)
  ...
}

// to use it:

useRequest({
  url: '/api/data',
  params: { id }
}, ['/api/data', id])

useRequest({
  url: '/api/data'
}, ['/api/data'])

Just like React's useMemo, useEffect, and useCallback.

But it's not an anti-pattern or "mistake" to use JSON.stringify here. I'm good with using it in this specific example and sometimes it is easier to use and working well for small apps indeed. That's why I think we need a comment in the example's code to clarify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like your suggestion. I'll try to update it with that 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quietshu How does useSWR react if there's no key? Or if you pass it an empty array? Will it return the cached version of a different call that also was created with an empty array? Or will those arrays be different arrays, and if so, how would it be cached if the array next render "changes" to a new empty array? 🤔

Copy link
Member

@shuding shuding Nov 25, 2019

Choose a reason for hiding this comment

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

@Svish SWR will "pause" the data fetching (not fetching anything and just return undefined as the data) if the key is an empty string, array or null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it wouldn't really work then, because you'd need something more to use as a key. An empty deps array, in the case of a query with no parameter, would be ignored, and an array with for example an id of 2, would potentially be mixed up with a different query which also had an id of 2 (for example a post and a user with the same id). 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@quietshu Hm, after looking at it more, I think it's easier to just use the JSON.stringify, because it's difficult to know exactly how much in the request object one should need to include in the array. And from what I can understand, JSON.stringify is fairly fast, and it should also be stable as long as the object is created the same way each time.

Would probably be good to add a comment about that though, as you mention, but not sure how it should be written. Maybe one of you guys could try to author something along the lines of what you think it should say, and I can add it to the code? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@Svish No worries, I think it's fine for now.

Choose a reason for hiding this comment

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

@Svish Hi. With a current approach, I stuck with an issue when I need to mutate some request but I don't know its params cause they are too high in a tree of components and there is no sense to put it down to children.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Svish Hi. With a current approach, I stuck with an issue when I need to mutate some request but I don't know its params cause they are too high in a tree of components and there is no sense to put it down to children.

Well, then you just need to do it differently then 😛 I'm currently only doing fetching with this library, so haven't done anything with mutate or such yet.

Choose a reason for hiding this comment

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

Seems like there is a sense to pay attention to the "apollo-client" implementation of working with cache (__typename and etc) cc @quietshu

Copy link
Member

@shuding shuding left a comment

Choose a reason for hiding this comment

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

Thank you for your effort!

@shuding shuding merged commit 335f425 into vercel:master Nov 27, 2019
@Svish Svish deleted the axios-examples branch December 6, 2019 09:38
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.

4 participants