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

fix(react-query): correct return type of useSuspendedQuery #73

Merged
merged 3 commits into from
Oct 25, 2022

Conversation

manudeli
Copy link
Member

@manudeli manudeli commented Oct 20, 2022

Overview

Related Issue #72

I corrected useSuspendedQuery return type when enabled options in useSuspendedQuery is boolean

When enabled in options of useSuspendedQuery is boolean, data outside of if(isSuccess) statement can be undefined

But, data inside of if(isSuccess) statement have to be TData, not TData | undefined

when enabled: boolean, I thought SuspendedUseQueryResultOnSuccess | SuspendedUseQueryResultOnIdle is right return type , not BaseSuspendedUseQueryResult

Incorrect: BaseSuspendedUseQueryResult<TData | undefined>
Expected: SuspendedUseQueryResultOnSuccess<TData> | SuspendedUseQueryResultOnIdle<undefined>

in use case

AS-IS

const Component = () => {
  const boolean = !!(Math.random() > 0.5)

  const query = useSuspendedQuery(
    ["example"] as const,
    async () => {
      return "response" as const
    },
    { enabled: boolean }
  )

  console.log(query.data) // BaseSuspendedUseQueryResult<"response" | undefined>.data: "response" | undefined

  if (query.isSuccess) {
      console.log(query.data) // BaseSuspendedUseQueryResult<"response" | undefined>.data: "response" | undefined
  }

  return ...
}

TO-BE

const Component = () => {
  const boolean = !!(Math.random() > 0.5)

  const query = useSuspendedQuery(
    ["example"] as const,
    async () => {
      return "response" as const
    },
    { enabled: boolean }
  )

  console.log(query.data) // BaseSuspendedUseQueryResult<"response" | undefined>.data: "response" | undefined

  if (query.isSuccess) {
      console.log(query.data) // BaseSuspendedUseQueryResult<"response">.data: "response"
  }

  return ...
}

How to correct

AS-IS

export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options?: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'>): SuspendedUseQueryResultOnSuccess<TData>;
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'> & {
    enabled?: true;
}): SuspendedUseQueryResultOnSuccess<TData>;
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'> & {
    enabled: false;
}): SuspendedUseQueryResultOnIdle<undefined>
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>): BaseSuspendedUseQueryResult<TData | undefined>;

TO-BE

export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options?: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'>): SuspendedUseQueryResultOnSuccess<TData>;
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'> & {
    enabled?: true;
}): SuspendedUseQueryResultOnSuccess<TData>;
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: Omit<SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>, 'enabled'> & {
    enabled: false;
}): SuspendedUseQueryResultOnIdle<undefined>
export declare function useSuspendedQuery<TQueryFnData = unknown, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey>(queryKey: TQueryKey, queryFn: QueryFunction<TQueryFnData, TQueryKey>, options: SuspendedUseQueryOptions<TQueryFnData, TError, TData, TQueryKey>): SuspendedUseQueryResultOnSuccess<TData> | SuspendedUseQueryResultOnIdle<undefined>;

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

@netlify
Copy link

netlify bot commented Oct 20, 2022

Deploy Preview for slash-libraries ready!

Name Link
🔨 Latest commit 948e6d6
🔍 Latest deploy log https://app.netlify.com/sites/slash-libraries/deploys/635794647fe928000917e27b
😎 Deploy Preview https://deploy-preview-73--slash-libraries.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@manudeli manudeli changed the title fix(@toss/react-query): correct return type of useSuspendedQuery fix(react-query): correct return type of useSuspendedQuery Oct 23, 2022
@changyoungoh
Copy link
Contributor

Thanks @manudeli !
Actually, I was planning to adopt tsd for testing complex types like this.

If you like, you can do this in this PR, or I will do it someday later.
What do you say?

@manudeli
Copy link
Member Author

Thanks @manudeli ! Actually, I was planning to adopt tsd for testing complex types like this.

If you like, you can do this in this PR, or I will do it someday later. What do you say?

I really want to add tsd's code for testing these types by myself👌~ if I can
but, How about new Pull Request to install tsd for whole monorepo like shopify/quilt and to test these types after this PR merging

I want to check your meaning

  1. Is it your meaning? that I can install SamVerschueren/tsd into whole monorepo dependencies of @toss/slash and setup tsd to test whole project like @shopify/quilt?

image

  1. If I can make new PR to set up tsd to this project, Could I make an issue before new Pull Request if I can make new PR?
    Actually I haven't setup or use tsd so I will take time to check how to set up tsd in this project. but when I checked tsd's document, if setup is finished, Maybe I can use tsd's grammer easily to test types

@changyoungoh
Copy link
Contributor

@manudeli
I think you're right about seprating to next PR.

And yes, you can open an issue, and you can setup up tsd tests in whole monorepo.
But with bottom-up approach,
how about adopting tsd only in @tossteam/react-query first and do the whole thing later?

@changyoungoh changyoungoh enabled auto-merge (squash) October 25, 2022 07:46
@manudeli
Copy link
Member Author

@manudeli I think you're right about seprating to next PR.

And yes, you can open an issue, and you can setup up tsd tests in whole monorepo. But with bottom-up approach, how about adopting tsd only in @tossteam/react-query first and do the whole thing later?

Ok, I will make an issue for plan to setup tsd on @toss/react-query first.

@changyoungoh changyoungoh merged commit ad534d1 into toss:main Oct 25, 2022
@manudeli manudeli mentioned this pull request Oct 28, 2022
1 task
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

2 participants