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

experimental use support for suspense #2781

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

experimental use support for suspense #2781

wants to merge 1 commit into from

Conversation

phryneas
Copy link
Member

This adds experimental use support. (For more context, see reactjs/rfcs#229)

If this would land like this, it could eliminate all the "when do we start the request" problems we are having and supersede #2245.

*/
then: useCallback<Promise<unknown>['then']>(
(onfulfilled, onrejected) => {
initiateQueryIfNeeded()
Copy link
Member Author

@phryneas phryneas Oct 16, 2022

Choose a reason for hiding this comment

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

The question is still open if React will throw away the current component state & refs when suspending before the component was rendered at least once.

If it would, this would not work (we would add new component subscriptions that would never be cleaned up) and we would have to try something with getRunningQueryThunk here instead. But let's hope that we won't have that problem in the future.

@codesandbox-ci
Copy link

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 3182636:

Sandbox Source
Vanilla Configuration
Vanilla Typescript Configuration
rsk-github-issues-example Configuration
@examples-query-react/basic Configuration
@examples-query-react/advanced Configuration
@examples-action-listener/counter Configuration

@phryneas
Copy link
Member Author

There are still open questions like "how would that look like with in-component query waterfalls?" and we would probably have to provide some kind of "memoizing Promise.all" for parallel suspending queries.

const { data1, data2, data3 } = use(memoPromiseAll({
  data1: useSomeData(arg1),
  data2: useSomeMoreData(arg2),
  data3: useEvenModeData(arg3)
}))

But maybe even use could come with something like that in the long run, so it might become

const { data1, data2, data3 } = use({
  data1: useSomeData(arg1),
  data2: useSomeMoreData(arg2),
  data3: useEvenModeData(arg3)
})

or a similar useAll hook:

const { data1, data2, data3 } = useAll({
  data1: useSomeData(arg1),
  data2: useSomeMoreData(arg2),
  data3: useEvenModeData(arg3)
})

@phryneas
Copy link
Member Author

What this also does not take into account right now is "what if that cache value refreshes". use will always reference that one promise - so maybe for that we would either have to trick around in the useQuery hook itself and wiggle some data from useQueryState in - or we would have to fall back to a separate implementation.

'Cannot refetch a query that has not been started yet.'
)
return promiseRef.current?.refetch()
return {
Copy link

Choose a reason for hiding this comment

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

I don't understand the entire code, but use will attach .value prop, so you want to keep useMemo maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, good point, that might be very tricky.

The return value of this hook will be spread in yet another hook, so having a stable value here will not help a lot - and that other value can be memoized, but will change more often since it has other "changing values" in it.

We would probably have to manage .value on our own in that context 🤔

Base automatically changed from v1.9-integration to master November 4, 2022 14:06
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