Suspense support for React hooks #215
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
airhorns
force-pushed
the
urql-suspense
branch
2 times, most recently
from
3f0ceba
to
99c6658
Compare
airhorns
force-pushed
the
urql-suspense
branch
2 times, most recently
from
6da171f
to
22fe891
Compare
airhorns
changed the title
|
ping @kristianpd @jasong689 |
jasong689
reviewed
Jun 29, 2023
| // use a memo as urql rerenders on context identity changes | ||
| const context = useMemo(() => { | ||
| return { | ||
| suspense: !!options?.suspense, |
There was a problem hiding this comment.
i'm guessing this keeps suspense off by default for all hooks
jasong689
approved these changes
Jun 29, 2023
React's new Suspense support is the perfect thing for components declaring that they need something to happen before they can fully render. With all the upcoming auth work that we are doing, I think we're going to have lots of situations on the frontend where we want to know current login state, but where we don't have it yet because its a GraphQL call away. We could solve this by wrapping the whole app in a thing that doesn't render until the session has been fetched. Thats what the `<ClerkProvider/>` does. I think we could do the same thing in our provider, but that'd require us assuming that every provider wants a session, which I don't think is a great assumption. Instead, I'd rather have the components that need to know login state block, but the ones that don't ... not! We can do that with suspense. If we add a `<RequireLogin/>` component, it can access session state with a suspense, and then when it arrives, actually decide what to do. It would use the normal, community-agreed-upon mechanism for showing intermediate state spinners or what have you as well. I think our app template should have a base-level suspense that shows a big spinner, but that's nice cause users can customize it or add suspense catchers farther down the tree. So, this adds suspense support to our React hooks! You can see it as an alternative to the `fetching` boolean. Instead of managing the fetching state, you punt it to your parent. See the tests for invocation examples. One annoyance with the way urql's suspense support works is that you have to turn suspense support on at the whole client level, and then individual hooks can choose to enable it or not. By default in urql, when you turn on suspense support in the client, all hooks start using it, and I don't think we want that, we just want folks to be able to use it if they want to. So, there's a bit of args munging to have suspense on at the client level so urql will do it at all, and then we opt-in on a per-hook basis.
kristianpd
approved these changes
Jun 30, 2023
airhorns
added a commit
that referenced
this pull request
Jul 6, 2023
… suspend by default In #215, we introduced automatic suspense support for the React query hooks, which allows developers (and us) to mark specific queries with "we want them to suspend" so that urql can do its thing under the hood and do so. We wanted this for a nice api in the `useSession` etc hooks. To implement this though, we need to turn urql's suspense support on at the client level. This is a constructor option which makes suspense available at all *and* makes it the default for all queries. So that this wasn't a breaking change for all users, I turned on suspense support in the client, but then made all queries pass an explicit `suspense: false` down to urql so the behaviour was unchanged between versions. If a query wants to suspend, it has to opt-in by passing `suspense: true`. I bungled this though, and did it for all the high level hooks like `useFindMany` and `useAction`, but I forgot that we export and make use of the low level `useGadgetQuery` hook for running manual GraphQL queries. Critically, we use this in the `@gadgetinc/react-shopify-app-bridge` package to detect installed status. The effect of this miss is that when the old react-shopify-app-bridge package is paired with a new api-client-core version, the bridge gets a client marked with `suspense: true` on the constructor, and then uses the low level `useGadgetQuery` hook, which doesn't apply the per-query default. The hook then suspends, which we learned breaks shopify installation.
airhorns
added a commit
that referenced
this pull request
Jul 6, 2023
…r react hooks where we know its supported This was a bad approach from #215. If we ship suspense on by default in api-client-core, it will be force-upgraded by Gadget regenerating packages on users *without* a react dependency bump at the same time, which means folks' clients will start suspending by default when used with old react hook versions. Oops -- thats a big breaking change that users don't have control over. Instead, we only have new versions of the react hooks turn on suspense mode by hacking it in manually.
airhorns
added a commit
that referenced
this pull request
Jul 6, 2023
…r react hooks where we know its supported This was a bad approach from #215. If we ship suspense on by default in api-client-core, it will be force-upgraded by Gadget regenerating packages on users *without* a react dependency bump at the same time, which means folks' clients will start suspending by default when used with old react hook versions. Oops -- thats a big breaking change that users don't have control over. Instead, we only have new versions of the react hooks turn on suspense mode by hacking it in manually.
airhorns
added a commit
that referenced
this pull request
Jul 6, 2023
… suspend by default In #215, we introduced automatic suspense support for the React query hooks, which allows developers (and us) to mark specific queries with "we want them to suspend" so that urql can do its thing under the hood and do so. We wanted this for a nice api in the `useSession` etc hooks. To implement this though, we need to turn urql's suspense support on at the client level. This is a constructor option which makes suspense available at all *and* makes it the default for all queries. So that this wasn't a breaking change for all users, I turned on suspense support in the client, but then made all queries pass an explicit `suspense: false` down to urql so the behaviour was unchanged between versions. If a query wants to suspend, it has to opt-in by passing `suspense: true`. I bungled this though, and did it for all the high level hooks like `useFindMany` and `useAction`, but I forgot that we export and make use of the low level `useGadgetQuery` hook for running manual GraphQL queries. Critically, we use this in the `@gadgetinc/react-shopify-app-bridge` package to detect installed status. The effect of this miss is that when the old react-shopify-app-bridge package is paired with a new api-client-core version, the bridge gets a client marked with `suspense: true` on the constructor, and then uses the low level `useGadgetQuery` hook, which doesn't apply the per-query default. The hook then suspends, which we learned breaks shopify installation.
airhorns
added a commit
that referenced
this pull request
Jul 6, 2023
…r react hooks where we know its supported This was a bad approach from #215. If we ship suspense on by default in api-client-core, it will be force-upgraded by Gadget regenerating packages on users *without* a react dependency bump at the same time, which means folks' clients will start suspending by default when used with old react hook versions. Oops -- thats a big breaking change that users don't have control over. Instead, we only have new versions of the react hooks turn on suspense mode by hacking it in manually.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
React's new Suspense support is the perfect thing for components declaring that they need something to happen before they can fully render. With all the upcoming auth work that we are doing, I think we're going to have lots of situations on the frontend where we want to know current login state, but where we don't have it yet because its a GraphQL call away.
We could solve this by wrapping the whole app in a thing that doesn't render until the session has been fetched. Thats what the
<ClerkProvider/>does. I think we could do the same thing in our provider, but that'd require us assuming that every provider wants a session, which I don't think is a great assumption.Instead, I'd rather have the components that need to know login state block, but the ones that don't ... not! We can do that with suspense. If we add a
<RequireLogin/>component, it can access session state with a suspense, and then when it arrives, actually decide what to do. It would use the normal, community-agreed-upon mechanism for showing intermediate state spinners or what have you as well. I think our app template should have a base-level suspense that shows a big spinner, but that's nice cause users can customize it or add suspense catchers farther down the tree.So, this adds suspense support to our React hooks! You can see it as an alternative to the
fetchingboolean. Instead of managing the fetching state, you punt it to your parent. See the tests for invocation examples.One annoyance with the way urql's suspense support works is that you have to turn suspense support on at the whole client level, and then individual hooks can choose to enable it or not. By default in urql, when you turn on suspense support in the client, all hooks start using it, and I don't think we want that, we just want folks to be able to use it if they want to. So, there's a bit of args munging to have suspense on at the client level so urql will do it at all, and then we opt-in on a per-hook basis.