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

Type fetcher function arguments based on array key (#331) #572

Closed
wants to merge 2 commits into from

Conversation

TLadd
Copy link

@TLadd TLadd commented Jul 30, 2020

Changes in tests provide and example of what this looks like:

const { data: v1 } = useSWR(
  ['args-1', obj, arr] as const,
  (a, b, c) => a + b.v + c[0]
)

Need to do as const so Typescript infers the tuple type instead of something like (string | object | string[])[] (in this case). Now, a, b, and c have the proper types and there are type errors if I misuse one of the variables or have the incorrect number of arguments.

@shuding shuding added the discussion Discussion around current or proposed behavior label Aug 20, 2020
@shuding
Copy link
Member

shuding commented Aug 20, 2020

Thanks for bringing this up! Do you think if there're any other work arounds that don't require as const? It feels a little bit overhead to me to have that.

@TLadd
Copy link
Author

TLadd commented Aug 20, 2020

I would have thought the as const was necessary for TypeScript to infer the correct type for the arguments tuple. By default, TypeScript sees something like

let args = ["hello", 42]

and infers a type of (string | number)[]. It does this because arrays are mutable and it allows me to call .push or .remove to mutate the items in the array. When I instead do

let args = ["hello", 42] as const

The as const tells TypeScript to make it a type error to mutate the array and thus it is safe to assign the tuple type readonly ["hello", 42] to args.

I looked into what react-query was doing and noticed https://github.com/tannerlinsley/react-query/blob/master/types/index.d.ts#L202. I don't really understand why, but this apparently forces TypeScript to infer the tuple when the array is passed as the argument directly (not assigned to a variable first). I copied that same type over and it works, except for the case when the key is a function where as const is still necessary.

Note I needed to upgrade TypeScript in order to allow for the circular type reference.

@shuding
Copy link
Member

shuding commented Oct 31, 2020

cc @promer94 would like to hear your thoughts about this

@promer94
Copy link
Collaborator

promer94 commented Nov 2, 2020

TypeScript V4 made some great improvements on tuple type inferences. I create a simplified useSWR in typescript playgroud.

ts-swr.gif

In this way, user dont have to use 'as const' to force typescript to infer tuples.

However, we have to solve some problems before we bring this feature into swr.

Stable api design

Since we are working on next version fo swr, some of apis may change.

Upgrade TypeScript Version

  1. upgrade TypeScript version
  2. upgrade eslint and prettier (so they could parse and format typescript correctly)
  3. refactor useSWR type (all the related types like cache interface also need refactor)
  4. may need to upgrade ts-jest and @testing-library/react

Backward compatibility

Some users may not be able to upgrade the typescript in their project. I don't have much experiences about this part.
https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html#version-selection-with-typesversions

In general, i think it would be great if swr could have this ability. I would love to do more researches and experiments on this.

@promer94
Copy link
Collaborator

promer94 commented Nov 2, 2020

cc @shuding @huozhi

@vhakulinen
Copy link

Any updates on this?

@pacocoursey pacocoursey removed their request for review July 9, 2021 18:19
@promer94 promer94 mentioned this pull request Sep 18, 2021
@shuding
Copy link
Member

shuding commented Sep 26, 2021

A solution without as const has been landed in #1477, thank y'all for the contributions and discussions here! 🎉

@shuding shuding closed this Sep 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion around current or proposed behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants