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

Optimize isValidating status #967

Merged
merged 13 commits into from
Mar 6, 2021
Merged

Optimize isValidating status #967

merged 13 commits into from
Mar 6, 2021

Conversation

anothertempore
Copy link
Contributor

This PR is set the initial isValidating to be true if there is a request will be sent on mount.

Currently, if we will fetching on mount, the isValidating will change like false -> true -> false. If we using isValidating to indicate a loading status, which is a bit weird.

I think it could be better to set the initial isValidating to true if we will start fetching. So it is true -> false.

If there is anything wrong, please correct me. 🙈 Thanks!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Feb 5, 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 269459b:

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

src/use-swr.ts Outdated Show resolved Hide resolved
@shuding
Copy link
Member

shuding commented Feb 8, 2021

Thanks! I don't like the false -> true -> false state change either but my only concern is that this might cause the hydration mismatch. But I'm almost sure there's a way to determine and avoid that. I'll review and think about this again carefully.

@chybisov
Copy link

I don't like the false -> true -> false either and hope this PR will be merged. @shuding did you have a chance to think about it? Thank you!

@LRNZ09
Copy link

LRNZ09 commented Feb 25, 2021

This would be very appreciated. It's odd to have the initial loading state set to false.

src/use-swr.ts Outdated Show resolved Hide resolved
@shuding
Copy link
Member

shuding commented Mar 1, 2021

Hi @chybisov @LRNZ09 @Kexin-Li! I think we can have this merged and release a beta version, once the above suggestion is merged 🙏

Thanks for your effort @Kexin-Li @huozhi! And I need to test it a bit in a large application before releasing a stable version, because this change is quite critical. But once landed, this is gonna help the performance a lot!

Also, we need to update the docs when we release it: https://swr.vercel.app/advanced/performance

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.

Thanks!

@shuding shuding merged commit cb4ee96 into vercel:master Mar 6, 2021
@shuding
Copy link
Member

shuding commented Mar 6, 2021

Released as 0.4.3-beta.2 🎉

This was referenced Mar 15, 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.

5 participants