Skip to content

remove overloads #2918

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

Closed
TkDodo opened this issue Nov 11, 2021 · 8 comments
Closed

remove overloads #2918

TkDodo opened this issue Nov 11, 2021 · 8 comments
Assignees
Labels
v4 wontfix This will not be worked on
Milestone

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 11, 2021

Proposed solution

- useQuery(key, fn, options)
+ useQuery({ querykey, queryFn, ...options })
- queryClient.invalidateQueries(key, filters, options)
+ queryClient.invalidateQueries(filters, options) // queryKey is part of `filters`

Also, provide a codemod to help with the transition

queryKey should then also be required in the options

We also want to change the useQueries api (see #2923) for the full discussion. I'm moving the api change here because it should be part of the codemod, too:

- useQueries([ query1, query2 ])
+ useQueries({ queries: [ query1, query2 ] })
@TkDodo TkDodo added the v4 label Nov 11, 2021
@TkDodo TkDodo added this to the v4 milestone Nov 11, 2021
@babycourageous
Copy link
Contributor

I will try to tackle this one! Never done a codemod, so depends what all that would entail.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 13, 2021

Thanks @babycourageous. FYI, I've updated the description, it now also contains the intended api change for useQueries. It doesn't have any overloads, but the changes should also be part of the codemod please.

@babycourageous
Copy link
Contributor

Just started to kick this one off over the weekend.
Removing the overloads for useQuery and making the queryKey a required option had some understandable trickle down effects, heheh. One such being other overloaded functions.

Before I go too far down the refactor rabbit hole, just want to start confirming a few with ya!

Would you like similar/related functions to follow the same single options object/no overloads api. For example, queryClient.prefetchQuery and queryClient.fetchQuery currently follows the same overload structure as v3 useQuery. I would refactor these to only accept options with the newly required `queryKey``

ie

  prefetchQuery<
    TQueryFnData = unknown,
    TError = unknown,
    TData = TQueryFnData,
    TQueryKey extends QueryKey = QueryKey
  >(
    options: FetchQueryOptions<TQueryFnData, TError, TData, TQueryKey>
  ): Promise<void> {
    return this.fetchQuery(options).then(noop).catch(noop)
  }

Now that queryKey is being required another problem is in queryClient.setQueryDefaults which currently accepts a key and defaultOptions. This could also follow a similar structure but would require a couple other changes

interface QueryDefaults {
  // queryKey: QueryKey <-- REMOVE since it is required as part of QueryOptions
  defaultOptions: QueryOptions<any, any, any>
}

setQueryDefaults and getQueryDefaults would then reflect this change by removing the key, options params and only accepting options since key is required.

setQueryDefaults(
    options: QueryObserverOptions<any, any, any, any>
  ): void {
    const result = this.queryDefaults.find(
      x => hashQueryKey(options.queryKey) === hashQueryKey(x.defaultOptions.queryKey)
    )
    if (result) {
      result.defaultOptions = options
    } else {
      this.queryDefaults.push({
        defaultOptions: options,
      })
    }
  }


  getQueryDefaults(
    queryKey?: QueryKey
  ): QueryObserverOptions<any, any, any, any, any> | undefined {
    return queryKey
      ? this.queryDefaults.find(x => partialMatchKey(queryKey, x.defaultOptions.queryKey))
          ?.defaultOptions
      : undefined
  }

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 22, 2021

I've outline all the changes that I think should happen in this issue:

please have a look at the description and expand the details for examples (some of them) and syntax changes.

judging from that list:

  • fetchQuery and prefetchQuery fall in the same bucket as useQuery, so yes, we need the same changes there, too.
  • getQueryDefaults and setQueryDefaults are a different bucket: We want to get or set the defaults for a specific key - I don't think that they need to change because they have no overloads. This is similar to setQueryData - also accepts a key as first argument, and this is not going to change.

if its easier conceptually, let's split the "remove overloads" part and the "queryKey must be required part". In the end, queryKey is not going to be required everywhere. On filters for example, it won't be. I also don't understand the getQueryDefaults / setQueryDefaults part. Those functions look fine and I don't see why queryKey would need to be removed from interface QueryDefaults. It's also weird that this is apparently valid right now:
queryClient.setQueryDefaults('myKey', { queryKey: 'notMyKey'}) 🤷 . Also keep in mind that setQueryDefaults works fuzzily, so I really think it should stay out of the default options here, which would make the interface become:

interface QueryDefaults {
  queryKey: QueryKey
  defaultOptions: Omit<QueryOptions<any, any, any>, 'queryKey'>
}

hope that makes sense. If not, I'm happy to chat synchronously on a call about this :)

@babycourageous
Copy link
Contributor

Awesome, thanks!
Yah- all this makes perfect sense.

Totally agree on your buckets.

Seeing it typed out I think I even had that Omit<> version originally hah. I might have gotten an error somewhere and decided to try something different heheh.

Just let me know if you prefer splitting this into two PRs - overloads and required queryKey. I'm cool with whichever ya feel fit.

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 22, 2021

Since we likely can’t work in parallel, splitting is not needed unless doing it at once would make it harder for you :)

@babycourageous
Copy link
Contributor

Altogether is AOK (and how i started heheh)

@TkDodo
Copy link
Collaborator Author

TkDodo commented Dec 1, 2021

We’re not going forward with this as of now due to this design limitation in TypeScript: microsoft/TypeScript#43371 (comment)

@TkDodo TkDodo closed this as completed Dec 1, 2021
@TkDodo TkDodo added the wontfix This will not be worked on label Dec 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v4 wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants