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

feat(solid-query): SSR streaming support #4840

Merged
merged 20 commits into from
Feb 17, 2023

Conversation

marbemac
Copy link
Contributor

@marbemac marbemac commented Jan 19, 2023

Working off of the progress in #4803.

This refactors createBaseQuery to an alternative approach that consolidates to a single createResource call. Instead of coordinating two createResource calls - one that sends down the dehydrated query state, and one that sends down the query data - this PR uses a single createResource that tracks the entire query state, and leverages solid's native onHydrate functionality to hydrate that state on the client. This means we also get support for SSR streaming (and optional defer for specific requests just by setting the deferStream option)!

I also expanded the solid-start-streaming example to better showcase the various ssr patterns (and make it easier to experiment in real world).

ardeora and others added 7 commits January 12, 2023 06:00
Changed query checkers to use `isInitialLoading`

Fix hydration key bug for streaming SSR

Add `deferStream` option for streaming SSR
Store the entire query state in the resource, so that we can leverage native resource hydration to hydrate the query on the client as it’s streamed in.
Comment on lines 129 to 137
hydrate(queryClient(), {
queries: [
{
queryKey: defaultedOptions.queryKey,
queryHash: defaultedOptions.queryHash,
state: info.value,
},
],
})
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I'm assuming that hydrating mutations from server isn't really a use case - or is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah no need to hydrate the mutations here! Looks good


onCleanup(() => unsubscribe())
onCleanup(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Observing some strange behavior on the server with onCleanup. Pop a log in here, and load one of the pages that contains two queries... note how onCleanup is only triggered once on the server, despite two queries. Not sure why.

Copy link
Contributor

Choose a reason for hiding this comment

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

The PR I just created will solve this issue

})()
})
if (!unsubscribe) {
unsubscribe = createClientSubscriber(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For queries that were created in SSR, onHydrate will be called on the client, at which point we can hydrate the queryClient for that particular query, and then lazy subscribe to the observer.

}
resolve(unwrap(state.data))
Copy link
Contributor Author

@marbemac marbemac Jan 19, 2023

Choose a reason for hiding this comment

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

TBH, wasn't sure when/where/why to unwrap versus not unwrap... I understand what unwrap does, but any rules of thumb re where we need to use it?

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 19, 2023

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 43d3fb1:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@@ -1579,7 +1579,6 @@ describe('createQuery', () => {

// Fetch query
expect(states[0]).toMatchObject({
data: undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it important that this property be defined on the original state object (e.g. when we states.push({ ...state }))? When accessing through the proxy, e.g. state.data, it will come back as undefined as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

This fine for now! I think libraries built on top of solid-query would need to know about this caveat. That's worth a consideration

@marbemac marbemac marked this pull request as ready for review January 19, 2023 20:29
@ardeora
Copy link
Contributor

ardeora commented Jan 19, 2023

Thanks for working on this! I really like it. Any idea why refetchOnWindowFocus doesn't seem to work?

… just one possible approach
@TanStack TanStack deleted a comment from marbemac Jan 20, 2023
@ardeora
Copy link
Contributor

ardeora commented Jan 20, 2023

Sorry I dont know what just happened. I accidentally edited your comment. But take a look at the codesandbox example created above! Doesn't seem to work on my device :/

Screen.Recording.2023-01-19.at.10.52.12.PM.mov

@marbemac
Copy link
Contributor Author

Ah, maybe that behavior is due to 096da93? I branched off of your ssr branch, and then merged in latest v5 before starting my work. Perhaps trying switching back and forth between browser tabs to see if the refetch on visibility change occurs?

@ardeora
Copy link
Contributor

ardeora commented Jan 20, 2023

Ah, maybe that behavior is due to 096da93? I branched off of your ssr branch, and then merged in latest v5 before starting my work. Perhaps trying switching back and forth between browser tabs to see if the refetch on visibility change occurs?

That seems to be it! If I change tabs I can see it refetch on coming back to the tab

Comment on lines 69 to 77
const createClientSubscriber = (refetch: (info?: unknown) => void) => {
return observer.subscribe((result) => {
notifyManager.batchCalls(() => {
const unwrappedResult = { ...unwrap(result) }
setState(unwrappedResult)
refetch()
})()
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const createClientSubscriber = (refetch: (info?: unknown) => void) => {
return observer.subscribe((result) => {
notifyManager.batchCalls(() => {
const unwrappedResult = { ...unwrap(result) }
setState(unwrappedResult)
refetch()
})()
})
}
const createClientSubscriber = (refetch: (info?: unknown) => void, hydrated: boolean = false) => {
if (hydrated) {
defaultedOptions.refetchOnMount = false
observer.setOptions(defaultedOptions)
}
return observer.subscribe((result) => {
notifyManager.batchCalls(() => {
const unwrappedResult = { ...unwrap(result) }
setState(unwrappedResult)
refetch()
})()
})
}

Can we also add this to the client subscriber? This would allow us to not refetch a query that was rendered on the server, even if a staleTime wasn't set. Similar to how CSR apps behave

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure - I'll give it a try now. Would also be nice if we could figure out an actual "ssr" test that covers that hydrate code branch, and this stale behavior. Will see if I can figure that out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done in 9fc9e3d. Haven't figured out the ssr test... was looking for inspiration, but can't even find tests in the solidjs repo that cover ssr related functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great work! I can add the tests for SSR in a separate PR. Could you resolve the conflicts before I run the CI workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (v5@8c373d2). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@          Coverage Diff          @@
##             v5    #4840   +/-   ##
=====================================
  Coverage      ?   90.76%           
=====================================
  Files         ?       85           
  Lines         ?     3488           
  Branches      ?      881           
=====================================
  Hits          ?     3166           
  Misses        ?      301           
  Partials      ?       21           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ardeora
Copy link
Contributor

ardeora commented Jan 20, 2023

@TkDodo @marbemac made some awesome additions to createBaseQuery and this should be a good way to integrate both CSR and SSR in solid-query! Would love it if you could review it and merge the changes whenever convenient 😄 I'll close my PR once this gets merged

@ardeora ardeora requested a review from TkDodo January 20, 2023 23:47
@vercel
Copy link

vercel bot commented Feb 1, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated
query ⬜️ Ignored (Inspect) Feb 3, 2023 at 4:22PM (UTC)

@ardeora
Copy link
Contributor

ardeora commented Feb 3, 2023

@marbemac I made a PR with some fixes with this approach.

The PR fixes two issues:

  1. If we just use SSR (no Streaming) with Solid Start, when the query is hydrated, The data is updated due to it being a part of the resource. But the state store will be in its initial state. Where stuff like isFetching will be true, dataUpdatedAt will be 0, etc. This happens because there is no reactivity on the server! So even though we update the state store in the server subscription handler. The state variable never updates on server rendered DOM elements. So after the query is hydrated we can safely update the store
  2. The second issue will be noticed if we have two queries with the same key but one of them has initialData set. When the second query mounts, it will already have data set but it will still wait for the fetcher to resolve instead of deduping the data.

This also was the reason why the unsubscribe function wasn't working correctly on the server. It is fixed now.

@marbemac
Copy link
Contributor Author

marbemac commented Feb 3, 2023

@ardeora great! I'll take a look in a few hours.

@TkDodo TkDodo requested a review from ardeora February 12, 2023 20:36
@ardeora
Copy link
Contributor

ardeora commented Feb 14, 2023

Hello @marbemac! Could you rebase again from the v5 branch? I'll create another PR to fix the svelte-query tests then :D If they haven't been fixed already and get this PR merged

@marbemac
Copy link
Contributor Author

Sure thing! Can get that done today.

# Conflicts:
#	packages/solid-query/src/__tests__/createQuery.test.tsx
#	packages/solid-query/src/createBaseQuery.ts
#	pnpm-lock.yaml
@marbemac
Copy link
Contributor Author

@ardeora should be all set

@vercel
Copy link

vercel bot commented Feb 16, 2023

Deployment failed with the following error:

Creating the Deployment Timed Out.

@ardeora ardeora merged commit e87d483 into TanStack:v5 Feb 17, 2023
@ardeora
Copy link
Contributor

ardeora commented Feb 17, 2023

@marbemac Thanks again for all the hard work on this. The changes have been merged now. v5 is going to be 💯

@marbemac marbemac deleted the solid-query-streaming-ssr branch February 17, 2023 20:00
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.

None yet

4 participants