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

refactor: throw type error when skipToken is present in suspense query #8082

Merged
merged 24 commits into from
Oct 11, 2024

Conversation

gwansikk
Copy link
Contributor

@gwansikk gwansikk commented Sep 22, 2024

Summary

related: #8078 (reply in thread)

I improve DX(developer experience) by displaying an error during TypeScript type checking to indicate that skipToken cannot be used in Suspense Query.

Details

  • I re-declared queryFn in the options for Suspense Query.
  • Added overloaded types for Suspense Query to queryOptions and infiniteQueryOptions.

I removed the skipToken type from Suspense Query and modified the types as follows to display an error when skipToken is assigned.

// UseSuspenseQueryOptions (types.ts)
export interface UseSuspenseQueryOptions<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
> extends OmitKeyof<
    UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>,
    'queryFn' | 'enabled' | 'throwOnError' | 'placeholderData'
  > {
  queryFn: Exclude<
    UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>['queryFn'],
    SkipToken
  >
}

While it was a good attempt, I couldn't pass the options using queryOptions (because of the differing types), so I had no choice but to add a new overload for Suspense Query in queryOptions.

// New overload type for suspense query (queryOptions.ts)
export type UnusedSkipTokenOptions<
  TQueryFnData = unknown,
  TError = DefaultError,
  TData = TQueryFnData,
  TQueryKey extends QueryKey = QueryKey,
> = OmitKeyof<
  UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>,
  'queryFn'
> & {
  queryFn: Exclude<
    UseQueryOptions<TQueryFnData, TError, TData, TQueryKey>['queryFn'],
    SkipToken
  >
}

Copy link

nx-cloud bot commented Sep 22, 2024

☁️ Nx Cloud Report

CI is running/has finished running commands for commit f9aa558. 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 2 targets

Sent with 💌 from NxCloud.

Copy link

pkg-pr-new bot commented Sep 22, 2024

Open in Stackblitz

More templates

@tanstack/angular-query-devtools-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-devtools-experimental@8082

@tanstack/angular-query-experimental

pnpm add https://pkg.pr.new/@tanstack/angular-query-experimental@8082

@tanstack/eslint-plugin-query

pnpm add https://pkg.pr.new/@tanstack/eslint-plugin-query@8082

@tanstack/query-async-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-async-storage-persister@8082

@tanstack/query-broadcast-client-experimental

pnpm add https://pkg.pr.new/@tanstack/query-broadcast-client-experimental@8082

@tanstack/query-core

pnpm add https://pkg.pr.new/@tanstack/query-core@8082

@tanstack/query-devtools

pnpm add https://pkg.pr.new/@tanstack/query-devtools@8082

@tanstack/query-persist-client-core

pnpm add https://pkg.pr.new/@tanstack/query-persist-client-core@8082

@tanstack/query-sync-storage-persister

pnpm add https://pkg.pr.new/@tanstack/query-sync-storage-persister@8082

@tanstack/react-query

pnpm add https://pkg.pr.new/@tanstack/react-query@8082

@tanstack/react-query-devtools

pnpm add https://pkg.pr.new/@tanstack/react-query-devtools@8082

@tanstack/react-query-next-experimental

pnpm add https://pkg.pr.new/@tanstack/react-query-next-experimental@8082

@tanstack/react-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/react-query-persist-client@8082

@tanstack/solid-query

pnpm add https://pkg.pr.new/@tanstack/solid-query@8082

@tanstack/solid-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/solid-query-persist-client@8082

@tanstack/solid-query-devtools

pnpm add https://pkg.pr.new/@tanstack/solid-query-devtools@8082

@tanstack/svelte-query

pnpm add https://pkg.pr.new/@tanstack/svelte-query@8082

@tanstack/svelte-query-devtools

pnpm add https://pkg.pr.new/@tanstack/svelte-query-devtools@8082

@tanstack/svelte-query-persist-client

pnpm add https://pkg.pr.new/@tanstack/svelte-query-persist-client@8082

@tanstack/vue-query

pnpm add https://pkg.pr.new/@tanstack/vue-query@8082

@tanstack/vue-query-devtools

pnpm add https://pkg.pr.new/@tanstack/vue-query-devtools@8082

commit: f9aa558

@gwansikk
Copy link
Contributor Author

@TkDodo To implement this, I had no choice but to add a new overload type to queryOptions (modifying queryOptions feels risky). Is there a better way to handle this?

Copy link

codecov bot commented Sep 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.89%. Comparing base (2fe16e5) to head (f9aa558).
Report is 26 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main    #8082       +/-   ##
===========================================
+ Coverage   45.41%   81.89%   +36.48%     
===========================================
  Files         200       26      -174     
  Lines        7456      359     -7097     
  Branches     1696       98     -1598     
===========================================
- Hits         3386      294     -3092     
+ Misses       3694       56     -3638     
+ Partials      376        9      -367     
Components Coverage Δ
@tanstack/angular-query-devtools-experimental ∅ <ø> (∅)
@tanstack/angular-query-experimental ∅ <ø> (∅)
@tanstack/eslint-plugin-query ∅ <ø> (∅)
@tanstack/query-async-storage-persister ∅ <ø> (∅)
@tanstack/query-broadcast-client-experimental ∅ <ø> (∅)
@tanstack/query-codemods ∅ <ø> (∅)
@tanstack/query-core ∅ <ø> (∅)
@tanstack/query-devtools ∅ <ø> (∅)
@tanstack/query-persist-client-core ∅ <ø> (∅)
@tanstack/query-sync-storage-persister ∅ <ø> (∅)
@tanstack/react-query 93.12% <100.00%> (ø)
@tanstack/react-query-devtools 10.00% <ø> (ø)
@tanstack/react-query-next-experimental ∅ <ø> (∅)
@tanstack/react-query-persist-client 100.00% <ø> (ø)
@tanstack/solid-query ∅ <ø> (∅)
@tanstack/solid-query-devtools ∅ <ø> (∅)
@tanstack/solid-query-persist-client ∅ <ø> (∅)
@tanstack/svelte-query ∅ <ø> (∅)
@tanstack/svelte-query-devtools ∅ <ø> (∅)
@tanstack/svelte-query-persist-client ∅ <ø> (∅)
@tanstack/vue-query ∅ <ø> (∅)
@tanstack/vue-query-devtools ∅ <ø> (∅)

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 1, 2024

Is there a better way to handle this?

don’t think so

@gwansikk
Copy link
Contributor Author

gwansikk commented Oct 3, 2024

I believe this PR is ready for review, and I would appreciate your feedback.

  • I think this improvement has a significant impact on developer experience.
    Instead of displaying messages via console logs, it now shows errors through type checks.
  • For third-party packages using the React Query package, type safety is ensured.
    This allows third-party packages to be aware of the use of SkipToken in Suspense Hooks in advance, providing clearer type guarantees for development.

@gwansikk gwansikk marked this pull request as ready for review October 3, 2024 04:41
@saul-atomrigs
Copy link
Contributor

This is great, although I think we may keep the console.errors for those projects not using Typescript 😭
How about keeping the console.errors and adding the type checking altogether?

@gwansikk gwansikk changed the title refactor: handle type error when skipToken is present in suspense query refactor: throw type error when skipToken is present in suspense query Oct 6, 2024
@TkDodo
Copy link
Collaborator

TkDodo commented Oct 9, 2024

sorry about the conflicts - can you fix them please?

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 11, 2024

one more question: what happens if you pass options created queryOptions to useSuspenseQuery now?

const options = queryOptions({ queryKey: ['test'], queryFn: skipToken })

const query = useSuspenseQuery(options)

?

@gwansikk
Copy link
Contributor Author

one more question: what happens if you pass options created queryOptions to useSuspenseQuery now?

image

if skipToken exists in queryFn, a type error will occur.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 11, 2024

perfect - can you add a test for that too please? Then I'll ship it 🚢

@gwansikk
Copy link
Contributor Author

gwansikk commented Oct 11, 2024

perfect - can you add a test for that too please? Then I'll ship it 🚢

I have added test cases for queryOptions, infiniteQueryOptions!


There are no test errors in the local environment, but errors occasionally occur during the GitHub CI process. I’m unsure what to do to fix this error. Could you please help me? @TkDodo

Task logs: https://cloud.nx.app/logs/bCPDeKeWGE

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 11, 2024

I’ve tried to stabilize the test in a different PR. It’s just flaky right now.

@TkDodo TkDodo merged commit a991d92 into TanStack:main Oct 11, 2024
5 checks passed
@gwansikk gwansikk deleted the react-query/exclude-suspense-skiptoken branch October 11, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants