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

Is loading #284

Closed
wants to merge 2 commits into from
Closed

Is loading #284

wants to merge 2 commits into from

Conversation

ivos
Copy link
Contributor

@ivos ivos commented May 13, 2022

PR for #208

Comments:

  1. Some cases of setting isLoading = false are not covered by tests. I haven't found any test cases that would cover that for isValidating which I could extend. These are is a separate (second) commit.
  2. If this gets merged, SWRV docs site would need to be updated too.

@adamdehaven
Copy link
Member

@ivos is this still relevant after the Vue 2.7 upgrade? I'm guessing it will need some modifications if it's still something you're interested in adding

@ivos
Copy link
Contributor Author

ivos commented Sep 25, 2022

@adamdehaven Not sure how does this PR relate to upgrade of Vue on the master branch. This PR is against the next branch. What am I missing?

@adamdehaven
Copy link
Member

Me looking at the wrong base branch 😅

@adamdehaven adamdehaven deleted the branch Kong:next October 11, 2022 22:21
@ivos
Copy link
Contributor Author

ivos commented Oct 12, 2022

Ahh, I was missing the fact that the next brach was destined to be dropped instead of merged into master, obviously.

So now what? Should I create this PR once again, this time against the master branch? Is it even worth it? Is anyone going to merge it?

@adamdehaven
Copy link
Member

I read through the thread on #208 and it mostly makes sense, although I need to play with the Code Sandbox a little to get my head around what is being suggested.

If you think this is worth pursuing and would like to put up a PR against the master branch I can take a look in the next couple weeks. (Admittedly, I haven’t parsed through all of the open Issues/PRs yet)

@ivos
Copy link
Contributor Author

ivos commented Oct 12, 2022

Just a short recap.

The docs here https://docs-swrv.netlify.app/configuration.html states that "Return value data contains either data for the given key resolved by fetcher or undefined if not loaded".

The above statement is not true.

When you switch from key value 1 to key value 2, swrv starts loading data for key value 2 but the data still contains data for key value 1. This effectively means that the data value does NOT correspond to the key value and neither is undefined as the docs claims.

What's more, there's even no way to find out that this is happening.

This PR tries to enable a way for the client to find out as specified here: #208 (comment).

@adamdehaven
Copy link
Member

Better yet, we could have a new return value named isLoading that would be true when there's a request ongoing and the data is either undefined or does not correspond to the current key.

That way users can show a spinner when either:

  1. data is undefined, when showing cached data for the previous key is better than the spinner.
  2. isLoading is true, when showing cached data for the previous key is undesirable.

My concern here is that a new return value of isLoading is a bit confusing and seems to conflict with the isValidating value which is true "if there's a request or revalidation loading"

@ivos
Copy link
Contributor Author

ivos commented Oct 12, 2022

Yep. I fully understand your concern. That's why I was trying to elicit some feedback in the #208 ticket.

isValidating is set when a request is ongoing (value might or might not be in cache).
isLoading is set when a request is ongoing for a value that is not in cache.

I do admit that there might be a better name for isLoading to distinguish it from isValidating. I am totally open to any suggestions.

I've chosen isLoading with the intention to use it as trigger for a spinner.

@adamdehaven
Copy link
Member

adamdehaven commented Oct 12, 2022

Going back to your original question, if it's not too much effort, would you want to create a new PR against master (and reference here so people can follow) and we could leave it open for a period of feedback? I believe you could link to #208 to reopen that thread.

@ivos
Copy link
Contributor Author

ivos commented Oct 12, 2022

Ok, will do. Thanks.

@ivos ivos deleted the is-loading branch October 16, 2022 09:23
@ivos ivos mentioned this pull request Oct 16, 2022
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.

2 participants