fix(solid-query): respect suspense: false

[aadito123]

As of now, the default in solid-query is to suspend when accessing inside a suspense boundary. This may not always be ideal, hence the suspense option is provided. However, that option was not respected. Fixed now.

Default behaviour is still to suspend, but now with suspense: false it won't. Added relevant test case and updated comment to reflect the difference between react and solid's suspense behaviour.

#5657 This issue seems to have been a blocker for other users.

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)	Visit Preview		Dec 26, 2023 0:55am

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d9ad21e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx affected --targets=test:eslint,test:lib,test:types,test:build,test:bundle

---

Sent with 💌 from NxCloud.

[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 d9e4de1:

Sandbox	Source
@tanstack/query-example-angular-basic	Configuration
@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

All modified and coverable lines are covered by tests ✅

Comparison is base (499db13) 87.92% compared to head (d9e4de1) 88.34%.
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6415      +/-   ##
==========================================
+ Coverage   87.92%   88.34%   +0.42%     
==========================================
  Files          87       75      -12     
  Lines        2931     2592     -339     
  Branches      804      684     -120     
==========================================
- Hits         2577     2290     -287     
+ Misses        295      257      -38     
+ Partials       59       45      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[aadito123]

Nx shows that tests failed for react-query, unsure why.
Edit: Another solid test failed in CI. Can't get the same tests to fail locally and reliably. Usually all tests pass locally but 15% of the time, some fail and I can't recreate them. Assuming the tests are a bit flaky.

[TkDodo]

I'm not sure this is what we want. We've moved away from the suspense boolean flag in react, and as far as I remember talking to @ardeora, the suspense flag is unnecessary in solid. I think we should rather remove it completely on type level rather than starting to respect its value ...

[aadito123]

You are right. suspense: true is completely redundant in Solid, since the query will suspend out-of-the-box. However, there is no way to opt-out of suspense for a specific query. For example, just checking queryResult.isSuccess in the JSX would cause the component to suspend. I suppose it is fine if we remove the suspense flag but I think there should be some way to opt-out of suspense.

Another solution I can think of is to expose the .latest attribute of the underlying resource. For example, queryResult.latest.isSuccess. This would make it identical to Solid's createResource API which exposes the .latest to read values without suspending.

[TkDodo]

yeah in react, we have separate hooks to trigger suspense (useSuspenseQuery) or not (useQuery). If solid wants something similar, we have to discuss this.

But the suspense: boolean flag is not something we want to encourage moving forward. For example, we've already removed it on the QueryClient defaultOptions, so there is no way to set it globally anymore ...

[aadito123]

Actually, I do not think useSuspenseQuery should be introduced for a few reasons.

1. useQuery in Solid defaults to suspense. Introducing useSuspenseQuery would require useQuery to default to not suspend, which is a huge (annoying) change.
2. Suspense in Solid works differently than in React. React suspends the entire component, requiring the <Suspense /> boundary to be in a parent component. Solid suspends at the data access point. To illustrate:

// In React: 
function Comp() {
  const query = useSuspenseQuery(); // <-- suspense triggers here
  return <div>{query.data}</div>;
}
function Parent() {
  return (
    <Suspense fallback='No data'>  // <-- boundary has to be defined in Parent
      <Comp />
    </Suspense>
  );
}
// In Solid: 
function Comp() {
  const query = useQuery();
  return (
    <div>
      <Suspense fallback='No data'>  // <-- boundary can be defined at any point
        {query.data} // <-- suspense triggers here
      </Suspense> 
    </div>
  );
}

I think the adding a .latest would be ideal since it would not be a breaking change, nor would it affect default behaviours. The above code can then look like this:

function Comp() {
  const query = useQuery();
  return (
    <div>
        {query.latest.isSuccess ? query.latest.data : 'No data'} // <-- suspense never triggered
    </div>
  );
}

If this sounds good to you and @ardeora then I will just open another PR for it.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit d9e4de1. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx affected --targets=test:eslint,test:lib,test:types,test:build,test:bundle

---

Sent with 💌 from NxCloud.

[mary-ext]

Suspense in Solid works differently than in React.

Yeah, they work differently, and the query types would still have to have undefined, but aside from maintenance reasons there doesn't seem to be anything problematic usability-wise with having createSuspenseQuery

[Brendonovich]

I think the adding a .latest would be ideal since it would not be a breaking change, nor would it affect default behaviours. The above code can then look like this:

I would really like this. We've patched query locally to have latest be defined like this:

const latest = createMemo(() => {
  const val = queryResource.latest?.data
  return val !== undefined ? val : state.data
})

const handler = {
  get(
    target: QueryObserverResult<TData, TError>,
    prop: keyof QueryObserverResult<TData, TError> & "latest",
  ): any {
    if (prop === 'latest') return latest()

    const val = queryResource()?.[prop]
    return val !== undefined ? val : Reflect.get(target, prop)
  },
}

It could be changed to include the whole latest and state values, but just exposing data as would happen with latest on a regular createResource fits better in my head.

[Brendonovich]

Thinking on this more, I wonder if another approach would be for only data to trigger suspense in solid. It'd be nice if isLoading etc didn't trigger suspense, but accessing data itself did. Fairly different to the other solutions but I think it makes more sense.
I like to lean on isLoading for stuff like disabling inputs that can't be edited until data is loaded - something that can't be done using suspense since that would involve not showing the input entirely - but currently isLoading can trigger suspense, so I think only accessing data should do so.

[aadito123]

but currently isLoading can trigger suspense, so I think only accessing data should do so.

This seems like the right balance. @ardeora would love to have your seal of approval here.

[ardeora]

Hello @aadito123 ! Sorry for the late and absolutely unacceptable lack of response here. I wanted to get some of the improvements in place and rework the internals to have a better view of how we can respect the suspense: false flag.

There are a few things that making this work with createQuery difficult.

1. The primary one is handling errors with ErrorBoundary. Resources throw errors from inside where they are read. That means you can write the ErrorBoundary inside the same component where you created the query

const query = createQuery(() => ({...}))

return (
  return (
    <ErrorBoundary fallback={'Will be caught here when query errors'}>
       <p>{query.data}</p>
    </ErrorBoundary>
  )
)

I love this and honestly it makes writing components more intuitive but if someone sets suspense: false. What would happen here? We wont be sending the queryResource in the data property, so no way to throw errors from where the data is read. We would only be able to throw the error from where createQuery is called. Which means that ErrorBoundary would need to be on a component that is higher up in the tree

2. The only way SSR is supported right now is SolidJS is by using resources. During SSR, SolidJS waits for resources to resolve before they can flush the data. If suspense: false is used with SolidStart, SSR would fail catastrophically

So what's the correct API design here? I am open to feedback here but I feel we can adopt the react query approach. With v5 react-query introduced a separate useSuspenseQuery and useSuspenseInfiniteQuery API to do all things with suspense.

In SolidJS, Suspense always had first class support and is the recommended way to fetch data and perform async operations. So for Solid Query I feel like having a separate API for createNonSuspenseQuery and createNonSuspenseInfiniteQuery would be a good compromise.

This way there would be a clear distinction of what features are available with Non Suspense queries and how they differ from traditional queries. They would ideally be used client only and error boundaries would need to be higher up in the tree. What do you think? I would be happy to merge these new APIs if you feel they make sense.

Tagging @marbemac and @Brendonovich (apologies for the weekend spam) too for their feedback

[PeterDraex]

Now that isLoading is not triggering Suspense, maybe we don't need non-Suspense queries at all. Especially when they have so many drawbacks and gotchas.

Let's just remove the suspense config option.

[aadito123]

Hello @aadito123 ! Sorry for the late and absolutely unacceptable lack of response here. I wanted to get some of the improvements in place and rework the internals to have a better view of how we can respect the suspense: false flag.

No worries!

Agree with your point. I am not sure about the SSR part since I would assume that without suspense, the server just won't perform the query, and just send down the loading content.

Regardless, #7272 only triggers suspense for data when it is undefined. This seems like the right balance to me, so I dont think we need non-suspense query APIs.

On that note, I'll close this PR.

[Brendonovich]

@ardeora I think your recent changes address all the changes I'd make to suspense, last thing I'd like to see is a latest field for non-suspending access to the data, as I demoed in this comment.
Thanks for getting all these improvements out!

[aadito123]

query.isSuccess ? query.data : null should give you the data without suspending. Accessing data only suspends when data is undefined.

[Brendonovich]

@aadito123 Ah yeah good point, I forgot ts query is SWR by default as opposed to createResource needing latest