feat(suspense): return tuple for useSuspenseQuery and useSuspenseInfiniteQuery #5822
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 2c36aec. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
|
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 2c36aec:
|
| @@ -44,4 +44,6 @@ export function useSuspenseInfiniteQuery< | |||
| InfiniteQueryObserver as typeof QueryObserver, | |||
| queryClient, | |||
| ) as InfiniteQueryObserverSuccessResult<TData, TError> | |||
|
|
|||
| return [query.data, query] | |||
There was a problem hiding this comment.
I guess you want to make library user can rename guaranteed TData easily by using tuple returning like useState?
but I want to mention there is some trade offs.
- making difference of interface with
useQuerywill raise barrier to entry a bit foruseSuspenseQueryuser. - it is expected to cause a BREAKING CHANGE if we add the
enabled optionin minor version so thatTDatacannot be guaranteed.
| return [query.data, query] | |
| return query |
Could I know why you want to make this change more?
There was a problem hiding this comment.
it is expected to cause a BREAKING CHANGE if we add the enabled option in minor version so that TData cannot be guaranteed.
not breaking if introduced enabled is true by default (and thus data should always be defined)
There was a problem hiding this comment.
if we add enabled (which we likely won't), we would do so that not specifying enabled will still result in guaranteed TData, and only if you add it (and make it false), it will be undefined. We do the same with initialData.
This change is mainly about developer ergonomics. With a normal query, you usually want access to data, error, isLoading etc. because you need to handle those states. With useSuspenseQuery, you mainly want access to data, and not the rest of the query.
The entry barrier should be fine, as this is a new hook. Also, TypeScript helps :)
There was a problem hiding this comment.
it is expected to cause a BREAKING CHANGE if we add the enabled option in minor version so that TData cannot be guaranteed.
not breaking if introduced
enabledis true by default (and thus data should always be defined)
first of all, I don't think enabled option has been determined to be always true yet. if then, I think we shouldn't make returning type as tuple with hugging chance to make BREAKING CHANGE If enabled can accept a boolean
Also we also have below way that useQuery users are familiar with already too. Isn't it better If we want to access data directly?
const {data: post, ...others} = useSuspenseQuery({
queryKey: ['post', postId],
queryFn: () => fetchPost(postId),
})There was a problem hiding this comment.
to make BREAKING CHANGE If enabled can accept a boolean
not sure what enabled has to do here:
const [data] = useSuspenseQuery({
queryKey: ['foo'],
queryFn: () => Promise.resolve('foo')
})
data will be of type string here, while:
const [data] = useSuspenseQuery({
queryKey: ['foo'],
queryFn: () => Promise.resolve('foo'),
enabled: someCondition
})
data will be of type string | undefined
Also we also have below way that useQuery users are familiar with already too.
Implementation detail, but object spread has the disadvantage that it triggers getters on the result object, which interferes with tracked queries, so I usually advise against that: https://tkdodo.eu/blog/react-query-render-optimizations#tracked-queries
There was a problem hiding this comment.
Yeah, I worried about same point. if we make enabled option making ReturnType<typeof useSuspenseQuery>[0] can have type TData | undefined
All this cases cannot have advantage of isSuccess flag's type narrowing too.
post: string | undefined
but in this below case
we can have advantage of isSuccess flag's type narrowing
post: string
There was a problem hiding this comment.
@juliusmarminge anyway, I am a big fan of trpc! I am so glad to meet you. I know https://trpc.io/docs/client/react/suspense this one. but I want to keep useSuspenseQuery's return type as just object like useQuery's returning type.
and you can also check situation when we make a @tanstack/react-query's new feature, useSuspenseQuery accepting enabled option in useSuspenseQuery of @suspensive/react-query. so I just don't want to make this useSuspenseQuery meet BREAKING CHANGE because of type narrowing problem.
There was a problem hiding this comment.
The isSuccess check is a good point. However, I really doubt that we will add enabled to useSuspenseQuery. enabled has two use-cases:
- dependent queries. With
suspense, queries are always run in serial, because react suspends on the first query, then runs and continues on the second query. so our dependent queries example literally becomes:
// Get the user
const [user] = useSuspenseQuery({
queryKey: ['user', email],
queryFn: getUserByEmail,
})
const userId = user.id
// Then get the user's projects
const [projects] = useQuery({
queryKey: ['projects', userId],
queryFn: getProjectsByUser,
})
there is no enabled needed at all for this.
- disabling a query from running until a condition is met, like user-input
suspense evolves a lot component composition, so the lazy queries example could become:
function Todos() {
const [filter, setFilter] = React.useState('')
return (
<div>
// 🚀 applying the filter will enable and execute the query
<FiltersForm onApply={setFilter} />
{filter && <TodosTable filter={filter} />
</div>
)
}
function TodosTable({ filter }) {
const [data] = useSuspenseQuery({
queryKey: ['todos', filter],
queryFn: () => fetchTodos(filter),
})
}
There was a problem hiding this comment.
I understood why you don't care enabled. but yet I don't think returning tuple is good way to give better DX.
- Quite different interface with useQuery. Should we make useQuery return tuple too? It will make definitely type narrowing problem
- Alternative way is quite good too. (Are there any downsides to these ways?)
Alternative way is also good to rename it too.
it's same with useQuery. it's easy. what do tuple have good thing more?
const postQuery = useSuspenseQuery({
queryKey: ['post', postId],
queryFn: () => fetchPost(postId),
})
const post = postQuery.data // TDataconst {data: post} = useSuspenseQuery({
queryKey: ['post', postId],
queryFn: () => fetchPost(postId),
})
post // TDataWe can keep simmilar shape with useQuery
const postQuery = useQuery({
queryKey: ['post', postId],
queryFn: () => fetchPost(postId),
})
if(postQuery.isSuccess){
const post = postQuery.data // TData
}I think it's easier to have a similar shape if it has similar functions.
Are there any downsides of object returning way?
Codecov ReportPatch coverage has no change and project coverage change:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## beta #5822 +/- ##
==========================================
- Coverage 97.11% 92.14% -4.97%
==========================================
Files 50 19 -31
Lines 2391 242 -2149
Branches 706 53 -653
==========================================
- Hits 2322 223 -2099
+ Misses 67 19 -48
+ Partials 2 0 -2 ☔ View full report in Codecov by Sentry. |
|
I've decided to not move forward with this, because the equivalent changes to |
This change is not merged now, but I think it was a good idea. Thanks for reconsidering👍 |
No description provided.