Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

RFC: Allow fetching to be forced to true by exchanges #444

Closed
wants to merge 2 commits into from

Conversation

kitten
Copy link
Member

@kitten kitten commented Oct 8, 2019

This allows OperationResult to contain fetching: true which would force the useQuery hook to keep fetching: true although data has already been served by an exchange.

This is useful to indicate that more data is being fetched during cache-and-network. Hence the legacy fetching flag could still be reproduced by doing oldFetching = fetching && !data.

@kitten kitten marked this pull request as ready for review October 8, 2019 14:30
@kitten kitten requested review from JoviDeCroock and mxstbr October 8, 2019 14:30
Copy link
Collaborator

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

Personally I would really like stale (cache-and-network refetch) and partial (partial return + fetch) flags.

With fetching going to true the user can get confused because data is returned but fetching is true, this introduces a new case in many applications. This bypasses the point of cache-and-network being a data refresh when we want to show stale data. Many people will have if (fetching) return <Spinner /> in their code at this point.

I've discussed the point of cache-and-network above the following will elaborate on partial.
Partial data introducing fetching is also a bit hit and miss in my opinion since a GQL schema should be closer to what he user really sees so something nullable should also be nullable in the front-end I can't really see why that should introduce fetching.

TLDR: this is breaking

@kitten
Copy link
Member Author

kitten commented Oct 8, 2019

Yes, that's why this is still an RFC. I think we mainly have to validate:

  • is this breaking? (If we use fetching: true, then yes)
  • should this be a separate state, i.e. stale?
  • how granular are we considering cache-and-network and partial background fetches to be?
  • can we expect this state to expand even more?
  • should there be a proactive flag as discussed previously where we observe using a subscription when the Client is fetching? (potentially expensive and complex)

Edit: on the note of “is something just missing/empty on the client or server”, I also agree that this should be represented in the schema.

In the case of a list for instance, the field shouldn’t be null to indicate an empty list but instead just an empty array or a connection with an empty array of edges.

@lukasluecke
Copy link
Contributor

In my opinion, having separate stale and partial flags would be great (as @JoviDeCroock suggested). If someone wanted to "merge" them back into fetching, they could just wrap that in a custom hook or something.

@JoviDeCroock
Copy link
Collaborator

The nice thing about stale is that we could consolidate stale and partial to be the same thing since essentially they are both stale. That would actually be something I can live with since I don't want to overcomplicate our result a lot.

@kitten
Copy link
Member Author

kitten commented Oct 11, 2019

Will be replaced by a new PR with stale based on master

@kitten kitten closed this Oct 11, 2019
@kitten kitten deleted the feat/fetching-via-exchanges branch February 6, 2020 19:03
@kitten kitten restored the feat/fetching-via-exchanges branch February 6, 2020 19:03
@kitten kitten deleted the feat/fetching-via-exchanges branch February 6, 2020 19:03
@kitten kitten mentioned this pull request Mar 10, 2020
8 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants