fix(solid-query): ssr fixes

[marbemac]

### 1. Don't use onCleanup on the server

It's still not safe to rely on onCleanup on the server, so I've re-added the !isServer check for the time being. I was able to isolate + reproduce the onCleanup behavior, and opened an issue in the solidjs repo - we'll see what Ryan + team thinks -> solidjs/solid#1599.

Nevermind! Ryan fixed the solidjs/solid#1599 in solid v1.6.13 - I verified it solves the problem here in solid-query, and bumped the minimum solid-js version to 1.6.13.

Before

Any page with more than one query on it cannot be rendered in SSR (streaming or regular async render). Try firing up the example and loading http://127.0.0.1:3000/deferred - it will load indefinitely.

After

Routes with more than one query on them can be rendered in SSR.

2. Update to prefer the query resource value, if present

In particular, this makes it possible to rely on props like isFetching, etc, while rendering on the server. It also ensures that the query state on the client starts with the correct values (e.g. if the query was already fetched on the server, isFetched should start off as true on the client).

I'm still unsure how to add tests for SSR related behavior, so for now my best "testing" method has been to create examples and manually verify. I added a http://127.0.0.1:3000/hydration route to the solid-start-streaming example.

Before

Note the mis-match on values like isFetching - the server and initial render still think it is true, when in fact it is done fetching and should be false. Also.. isRefetching set to true across the board - weird.

After

Note how after these changes, the values on the server, and on initial render, are more along the lines of what one would expect.

I'm not sure if there is a better way to solve this issue, or if something else is going on that is better fixed in query-core. There is still one weird bit of behavior - on first refetch note how isFetched briefly is set to false. Overall though this change feels like a solid step forward.. just not QUITE there yet across the board.

---

In case @ardeora or anybody else wants to mess around with the behavior, here's he best way I've found to get the example going for local dev:

1. Set @tanstack/solid-query to workspace:* in the example package.json (https://github.com/TanStack/query/blob/alpha/examples/solid/solid-start-streaming/package.json#L18)
2. Set main in the solid-query package.json to "main": "src/index.js",. Remove the exports, module, and types properties.
3. pnpm i
4. cd examples/solid/solid-start-streaming && pnpm dev
5. Make changes to the solid-query package as needed, the example will hot-reload

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated
query	⬜️ Ignored (Inspect)			Mar 29, 2023 at 4:56PM (UTC)

[codesandbox-ci]

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 9a82950:

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

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (0028a14) 90.73% compared to head (a8beb48) 90.73%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #5093      +/-   ##
==========================================
- Coverage   90.73%   90.73%   -0.01%     
==========================================
  Files         110      110              
  Lines        3993     3992       -1     
  Branches     1000     1000              
==========================================
- Hits         3623     3622       -1     
  Misses        337      337              
  Partials       33       33              

Impacted Files	Coverage Δ
packages/solid-query/src/createBaseQuery.ts	67.94% <100.00%> (-0.41%)	⬇️

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.

[marbemac]

Bumped for this fix -> solidjs/solid@60f8624

[marbemac]

I'm not sure why there are so many changes in the lock file that don't seem related to the packages I bumped. All I did was run pnpm install after bumping. For reference, I'm on pnpm v7.25.0.

[ardeora]

Thank you so much for working on the example and the fix. This looks great! Just have a simple comment about void 0 being used. Everything else looks 🔥 Could we also rebase from the alpha branch for me to merge it to the alpha branch?

[ardeora]

@marbemac Also the isFetched being false on the first refetch is because createBaseQuery doesnt seem to set the dataUpdateCount property when dehydrating the query on the server. This property is not available on the queryObserver result so we might have to manually set it?

query/packages/query-core/src/queryObserver.ts

Line 573 in fe0ae80

isFetched: state.dataUpdateCount > 0 || state.errorUpdateCount > 0,

[marbemac]

Howdy @ardeora - apologies for the delay here! I was out for a bit, but will take a look today.

[marbemac]

@ardeora ah one thing I noticed that I forgot to mention that was causing some issues in the app I'm building. What was the reason to force throwErrors=true on the server (https://github.com/TanStack/query/blob/alpha/packages/solid-query/src/createBaseQuery.ts#L48)? Versus leaving control of this to the library consumer?

[marbemac]

Also the isFetched being false on the first refetch is because createBaseQuery doesnt seem to set the dataUpdateCount property when dehydrating the query on the server. This property is not available on the queryObserver result so we might have to manually set it?

Hmm, begs the question.. why is the query state that we're hydrating missing some properties? This it the first time I'm actually digging into query-core a bit, and there might be a larger issue?

I didn't realize that QueryObserverResult and QueryState are managed separately. Basically, we're hydrating the query and setting the state, which is expecting QueryState, to a QueryObserverResult object in https://github.com/TanStack/query/blob/alpha/packages/solid-query/src/createBaseQuery.ts#L149 (since observer.subscribe passes us a QueryObserverResult).

Should we be subscribing to the query cache instead (since it passes the current query, including state, to the listeners) https://github.com/TanStack/query/blob/alpha/packages/query-core/src/queryObserver.ts#L665?

Looking at QueryState, 9 of 12 properties overlap with QueryObserverResult.

interface QueryState<TData = unknown, TError = DefaultError> {
  data: TData | undefined // present on QueryObserverResult
  dataUpdateCount: number // NOT present on QueryObserverResult
  dataUpdatedAt: number // present on QueryObserverResult
  error: TError | null // present on QueryObserverResult
  errorUpdateCount: number // present on QueryObserverResult
  errorUpdatedAt: number // present on QueryObserverResult
  fetchFailureCount: number // present BUT with a different name (failureCount) on QueryObserverResult
  fetchFailureReason: TError | null // present BUT with a different name (failureReason) on QueryObserverResult
  fetchMeta: FetchMeta | null // NOT present on QueryObserverResult
  isInvalidated: boolean // NOT present on QueryObserverResult (perhaps is present but with a different name? not sure what this is)
  status: QueryStatus // PRESENT on QueryObserverResult
  fetchStatus: FetchStatus // PRESENT on QueryObserverResult
}

For the immediate issue at hand, the isFetching issue is indeed occurring because dataUpdateCount is not present on QueryObserverResult (interestingly, errorUpdateCount IS present on QueryObserverResult).

is the intent to ultimately simplify and consolidate QueryState into QueryObserverResult? Or does managing QueryState as a separate thing serve some other functional purpose? Huge caveat here that I probably don't know what I'm talking about, JUST getting into query-core codebase for the first time 😅.

For the time being, I'll see if I can get the actual state object hydrating (rather than QueryObserverResult).

[marbemac]

Was wondering why the typings are not catching this mismatch re passing QueryObserverResult where QueryState is expected.

@TkDodo should we consider typing the dehydratedState argument? If it had been typed we would have seen this issue much sooner -> https://github.com/TanStack/query/blob/alpha/packages/query-core/src/hydration.ts#L116

[marbemac]

This does indeed fix the isFetched hydration issue. It's hacky, but works for now until there's a better solution 🤷‍♂️.

[ardeora]

Thanks for the updates! Yeah this feels like a fine tradeoff for now!

[marbemac]

From what I can tell, this isn't doing anything. Removed to simplify and reduce the number of calls to queryObserver.createResult. LMK if there's a reason for this and I can revert np!

[ardeora]

I would have to test more but this is from the first iteration of creating the adapter. I dont think we would need for now

[marbemac]

Some typings were added in @types/babel__core that indicate these should be strings (https://github.com/DefinitelyTyped/DefinitelyTyped/pull/63927/files#diff-a4da813bfdc10bbb7d4d9ee9b811309e7e2a16c0484b9a334752db84be4b0f31).

[ardeora]

Also the isFetched being false on the first refetch is because createBaseQuery doesnt seem to set the dataUpdateCount property when dehydrating the query on the server. This property is not available on the queryObserver result so we might have to manually set it?

Hmm, begs the question.. why is the query state that we're hydrating missing some properties? This it the first time I'm actually digging into query-core a bit, and there might be a larger issue?

I didn't realize that QueryObserverResult and QueryState are managed separately. Basically, we're hydrating the query and setting the state, which is expecting QueryState, to a QueryObserverResult object in https://github.com/TanStack/query/blob/alpha/packages/solid-query/src/createBaseQuery.ts#L149 (since observer.subscribe passes us a QueryObserverResult).

Should we be subscribing to the query cache instead (since it passes the current query, including state, to the listeners) https://github.com/TanStack/query/blob/alpha/packages/query-core/src/queryObserver.ts#L665?

Looking at QueryState, 9 of 12 properties overlap with QueryObserverResult.

interface QueryState<TData = unknown, TError = DefaultError> {
  data: TData | undefined // present on QueryObserverResult
  dataUpdateCount: number // NOT present on QueryObserverResult
  dataUpdatedAt: number // present on QueryObserverResult
  error: TError | null // present on QueryObserverResult
  errorUpdateCount: number // present on QueryObserverResult
  errorUpdatedAt: number // present on QueryObserverResult
  fetchFailureCount: number // present BUT with a different name (failureCount) on QueryObserverResult
  fetchFailureReason: TError | null // present BUT with a different name (failureReason) on QueryObserverResult
  fetchMeta: FetchMeta | null // NOT present on QueryObserverResult
  isInvalidated: boolean // NOT present on QueryObserverResult (perhaps is present but with a different name? not sure what this is)
  status: QueryStatus // PRESENT on QueryObserverResult
  fetchStatus: FetchStatus // PRESENT on QueryObserverResult
}

For the immediate issue at hand, the isFetching issue is indeed occurring because dataUpdateCount is not present on QueryObserverResult (interestingly, errorUpdateCount IS present on QueryObserverResult).

is the intent to ultimately simplify and consolidate QueryState into QueryObserverResult? Or does managing QueryState as a separate thing serve some other functional purpose? Huge caveat here that I probably don't know what I'm talking about, JUST getting into query-core codebase for the first time 😅.

For the time being, I'll see if I can get the actual state object hydrating (rather than QueryObserverResult).

One reason why they both are not exactly the same, because things like placeholderData or using the select function (to transform data) will only affect the data on the observer level. The queryState data is not always equal to the data on the QueryObserverResult.

[marbemac]

Hmm yeah I can see why query result and query state might each manage their own data properties. Still not clear why most of the other properties are duplicated between the two, some with slightly different names. A topic for another time though, thanks for merging!