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

chore: add a comment why fetcher accepts null #918

Merged
merged 1 commit into from
Jan 26, 2021

Conversation

koba04
Copy link
Collaborator

@koba04 koba04 commented Jan 20, 2021

UPDATED This PR adds a comment to describe why fetchFn accepts null.


This PR removes null from the type definition for fetchFn, but please correct me if I'm wrong.
I couldn't find any documentation about the behavior for passing null as a fetcher function. A case with undefined is documented though.
I also couldn't find any implementation for the case.

Is SWR supporting the case? If so, what is the expected behavior for the case?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 20, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ba9c670:

Sandbox Source
SWR-Basic Configuration
SWR-States Configuration
SWR-Infinite Configuration

@promer94
Copy link
Collaborator

see #738 for more details

@koba04 koba04 changed the title types: remove null from fetcherFn<Data> chore: add a comment why fetcher accepts null Jan 20, 2021
@koba04
Copy link
Collaborator Author

koba04 commented Jan 20, 2021

@promer94
Thank you for pointing it out. I start realizing SWR is used for various use-cases not only data fetching from this and #917.

As you mentioned at #738, it's a very hacking way, so I think it should be provided as an alternative API if SWR needs the feature.
I understand I cannot remove the type definition, so I've updated this PR to add a comment for the intention.

@shuding
Copy link
Member

shuding commented Jan 21, 2021

Thanks @koba04 @promer94 ! It’s like a design debate on this specific API, there’re both pros and cons on having a default fetcher or not.

And I agree that we should have an alternative way to do this. Using SWR for local data management is helpful and we are doing that too, passing null is just weird.

@shuding
Copy link
Member

shuding commented Jan 21, 2021

I think a good way is to provide more helper hooks that are built on top of useSWR, with a good preset of configurations.

import useSWRState from 'swr/state'

...

const { data, mutate } = useSWRState('key', initialVal)

where useSWRState is:

useSWR(key, null, { initialData })

Just an idea, curious about what you guys think about it.

@koba04
Copy link
Collaborator Author

koba04 commented Jan 22, 2021

I didn't know the use-case to use SWR for shared data management, so I'm not sure whether SWR should have the API or not. If so, exposing swr/state makes sense to me 👍
But as the website says "SWR is a React Hooks library for remote data fetching", it seems to be out of scope for SWR. So I think SWR should consider adding the support very carefully.

@shuding
Copy link
Member

shuding commented Jan 26, 2021

But as the website says "SWR is a React Hooks library for remote data fetching", it seems to be out of scope for SWR. So I think SWR should consider adding the support very carefully.

Totally agree and thanks for the great reminder here! This is always the goal of this lib, to thoughtfully support more scenarios with better UX and great performance, with a minimal API surface. Let's consider that later carefully.

@shuding shuding merged commit 22ef8aa into vercel:master Jan 26, 2021
@koba04
Copy link
Collaborator Author

koba04 commented Jan 26, 2021

Thank you!

@koba04 koba04 deleted the remove-null-fetcher-type branch January 26, 2021 14:18
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.

3 participants