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

Add Updating to Cells #2641

Merged
merged 5 commits into from Jun 10, 2021
Merged

Add Updating to Cells #2641

merged 5 commits into from Jun 10, 2021

Conversation

jtoar
Copy link
Collaborator

@jtoar jtoar commented May 26, 2021

As of Apollo Client v3, the default fetchPolicy is cache-first. cache-first is great, but cache-and-network is better. Accordingly, it's what we default to. It comes with a hiccup though: cache-and-network gives you data if it's there (the "cache" part), then makes a GraphQL request to see if there's anything new ("and-network"). This means that the "result" Cells get (and use to figure out what to render) has data (usually an object/array) and loading = true. And if loading = true, that means Loading gets rendered. Because things go in order:

if (error) {
if (Failure) {
return <Failure error={error} {...queryRest} {...props} />
} else {
throw error
}
} else if (loading) {
return <Loading {...queryRest} {...props} />
} else if (data) {
if (typeof Empty !== 'undefined' && isEmpty(data)) {
return <Empty {...queryRest} {...props} />
} else {
return <Success {...afterQuery(data)} {...queryRest} {...props} />
}
} else {
throw new Error(
'Cannot render cell: GraphQL success but `data` is null'
)
}

This is what it looks like: https://s.tape.sh/jf6jLX7u. I'm throttling the network so that you can really see Loading render. Near the end, you can see that the PostsPage (which has the PostsCell) displays Loading... even though there's data in the cache. (The other pages also display loading even though they have data too!)

Much in the same vein as #1878, this PR fixes this by providing an "Updating" state to Cells: https://s.tape.sh/YHPMztQO. The Loading states are gone. But there's a lot more details to consider:

  • How do you opt out of this?
  • If we're adding another state, we should really refactor the logic into a reducer; it's getting hard to keep track of how we get to each state. Cells are already state machines, is it time to use Xstate? We also don't need a library for state machines. But it seems like it's time to write one explicitly.
    • For instance, what happens when data's empty (which would render Empty) and loading = true?
  • We talked about having Cells export another component, Updating, but will that cause the React tree to commit when it's swapping between Success and Updating and we'll end up with a terrible flash anyway? I.e., should Updating be a child of Success, so it just pops in and out?
  • The way things are going, it seems like Cells might eventually look like the useAuth context, which provides an interface (logIn, signOut, etc.) that auth providers adapt themselves to.

Any and all feedback is welcome and appreciated!

@jtoar jtoar added this to the next-release-candidate milestone May 26, 2021
@jtoar jtoar added this to In progress in Current-Release-Sprint via automation May 26, 2021
@jtoar jtoar changed the title Add Updating state to Cells Add Updating to Cells May 26, 2021
@cannikin
Copy link
Member

Thanks for this @jtoar! I don't have answers to any of those questions, I just want this merged right away so I can stop issuing refetchQueries all over the place to update queries that are already cached! I need and-network to do that for me!

@cannikin
Copy link
Member

Oh damn I just added this to my app with patch-package and it seems to be working! Just added export const Updating = Success to my cells and everything works the way it's supposed to! Little tough to tell locally since everything renders so fast but I'm gonna deploy now to confirm.

@jtoar
Copy link
Collaborator Author

jtoar commented May 26, 2021

@cannikin Having to export const Updating = Success if there's no difference between the two seems like something we should remove the need for. There was discussion in Redwood Maker's about just and/or also passing a prop to Success (loading = true basically; or something else?). But lemme know how it goes!

@cannikin
Copy link
Member

IT'S WORKING

@cannikin
Copy link
Member

After this PR merges we should just release it as 1.0, this is all we need. THANKS @jtoar!!!!!!!

@cannikin
Copy link
Member

@cannikin Having to export const Updating = Success if there's no difference between the two seems like something we should remove the need for. There was discussion in Redwood Maker's about just and/or also passing a prop to Success (loading = true basically; or something else?). But lemme know how it goes!

I'm so excited this is working I don't mind adding the Updating = Success line in there. haha As far as I remember we don't have anywhere else where we pass "hints" to Redwood for what to do with different components (like adding { loading: true } as an arg).

I'm not too grossed out about just generating cells with export const Updating = Success at the end with a single line comment explaining what it does (and a link to the docs with more in-depth explanation)... /cc @mojombo

@jtoar
Copy link
Collaborator Author

jtoar commented May 26, 2021

Here's another catch: does Apollo's refetch function (the one that comes from useQuery's return) cause loading to be set back to true?

No, not by default (https://www.apollographql.com/docs/react/data/queries/#inspecting-loading-states):

If you click the refetch button, you'll see that the component doesn't re-render until the new data arrives. What if we want to indicate to the user that we're refetching the photo?

The useQuery hook's result object provides fine-grained information about the status of the query via the networkStatus property. To take advantage of this information, we set the notifyOnNetworkStatusChange option to true so our query component re-renders while a refetch is in flight:

So we'd have to make our defaults:

 beforeQuery = (props) => ({ 
   variables: props, 
   fetchPolicy: 'cache-and-network', 
   nextFetchPolicy: 'cache-first', 
+  notifyOnNetworkStatusChange: true,
 }),

packages/web/package.json Outdated Show resolved Hide resolved
tsconfig.compilerOption.json Outdated Show resolved Hide resolved
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

Just some feedback here about the code! This is such an awesome feature! Thanks for working on this @jtoar

packages/web/src/components/createCell.tsx Outdated Show resolved Hide resolved
packages/web/src/components/createCell.tsx Outdated Show resolved Hide resolved
packages/web/src/components/createCell.tsx Outdated Show resolved Hide resolved
@Tobbe
Copy link
Member

Tobbe commented May 27, 2021

This is how we do loading state for one of our cells. Sorry it's a long post, but I just wanted to show a loading state that's not just a small loading spinner or a simple overlay. And I hope this is a pattern that we can still support even with the new <Updating> component.

Showing scoring labels for 1 - 4

image

Then clicking + to configure the labels for scoring 1 - 5 we show this while it's fetching the labels from the DB

image

And when it's done loading we finally get this

image

The way I've implemented it is to reuse the <Success> component for my loading state as well, and then internally in <Success> detecting if we're loading or not

export const beforeQuery = (props) => {
  return { variables: props, fetchPolicy: 'cache-first' }
}

export const QUERY = gql`
  query ConfigureScoringLabelsQuery($topScore: Int!, $teamId: Int!) {
    defaultLabels: defaultScoreLabelsForTopScore(
      topScore: $topScore
      teamId: $teamId
    ) {
      id
      score
      label
    }
  }
`

export const Loading = ({ topScore, scoreLabels, setScoreLabels }) => (
  <Success
    topScore={topScore}
    scoreLabels={scoreLabels}
    setScoreLabels={setScoreLabels}
  />
)

export const Success = ({
  topScore,
  scoreLabels,
  setScoreLabels,
  defaultLabels,
}) => {
  const labels = scoreLabels[topScore]
  const isLoading = labels === undefined

  const setLabels = (labels) => {
    scoreLabels[topScore] = labels
    setScoreLabels({ ...scoreLabels })
  }

As you can see I use cache-first, and then, from there, I manage my own "cache" by storing the labels in context (that's what setScoreLabels do). This works out great for me, because the default labels I fetch from the DB never (super rarely) changes, so it's safe to just get them once.

And the inputs are just done like this:

<input
  type="text"
  value={isLoading ? 'Loading...' : labels[topScore - 1 - i].label}
  disabled={isLoading}
  onChange={(event) => {
    labels[topScore - 1 - i].label = event.target.value
    setLabels([...labels])
  }}
  className="w-full"
/>

@Krisztiaan Krisztiaan mentioned this pull request May 31, 2021
@jtoar jtoar moved this from In progress to In progress (priority) in Current-Release-Sprint Jun 4, 2021
@jtoar jtoar added the release:breaking This PR is a breaking change label Jun 8, 2021
@jtoar jtoar marked this pull request as ready for review June 8, 2021 00:36
Copy link
Contributor

@peterp peterp left a comment

Choose a reason for hiding this comment

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

This looks great, nicely done @jtoar

@jtoar jtoar removed this from the next-release-candidate milestone Jun 8, 2021
@jtoar jtoar moved this from In progress (priority) to Ready to merge in Current-Release-Sprint Jun 8, 2021
@thedavidprice thedavidprice added this to the next-release-priority milestone Jun 9, 2021
@jtoar jtoar requested a review from peterp June 9, 2021 21:49
@jtoar
Copy link
Collaborator Author

jtoar commented Jun 9, 2021

@peterp I was about to merge this but I realized I'm not typing the updating prop (which is really just loading renamed); can you give it another look to confirm I did it correctly?

@jtoar jtoar moved this from Ready to merge to In progress (priority) in Current-Release-Sprint Jun 9, 2021
@peterp
Copy link
Contributor

peterp commented Jun 10, 2021

@jtoar That looks good to me, my only recommendation would be to add comments so that updating prop is explained.

Current-Release-Sprint automation moved this from In progress (priority) to Ready to merge Jun 10, 2021
@jtoar jtoar merged commit 6cfe430 into redwoodjs:main Jun 10, 2021
Current-Release-Sprint automation moved this from Ready to merge to Done Jun 10, 2021
@jtoar jtoar deleted the ds-add-updating-to-cells branch June 10, 2021 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:breaking This PR is a breaking change topic/cells
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

convo: apollo network-and-cache / loading vs refetching networkStatus
7 participants