Skip to content

feat: add typescript support #767

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

Merged
merged 1 commit into from
Jul 29, 2020

Conversation

boschni
Copy link
Collaborator

@boschni boschni commented Jul 17, 2020

Hi there! While working on the project as a newcomer I noticed it was challenging at times to know which type of arguments some functions accept, which properties should be available on certain objects or which type a certain value should have.

Some of this information can be gathered from the docs, some of it from the tests, and some of it from the type definition file. But the docs only document the public APIs and are (together with the types) not always in sync with the code. Which means spending time on reading and debugging the code itself, while not knowing for sure what the intended behaviour exactly is.

I think adding types to the code will help people understand the project more easily and provide more confidence when making changes because mistakes can be caught early. It would also reduce the number of type related issues and knowing the project is written in TypeScript can be a selling point for TypeScript users.

This PR is mostly intended to get the discussion going and to see what would be needed to add support for it. At first glance not much is needed, but there would be some work involved in typing the code and it could potentially uncover some bugs here and there.

Would you be interested in exploring this direction?

@vercel
Copy link

vercel bot commented Jul 17, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@tannerlinsley
Copy link
Collaborator

Welcome to the long line of people that have and will continue to try and convince me to use TS for OSS libraries :) I hope it works this time! Here's my standard thoughts when it comes to TS and more or less my requirements for migrating to TS:

  • I don't want any breaking changes to the code or removal of the type defs for the first attempt. It can and should be a gradual and augmentative process. As the internal types get better with time, we can decide when the right time is to replace the type definitions with the actual generated types from the library.
  • I will always prefer any, unknown or something flexible (and maybe not 100% type-safe) before changing the dynamism of the code in a way that would make it less flexible than it already is. TS is notorious for putting restrictions on API design. Usually this is a good thing, but not always.
  • I don't want it to be intimidating or difficult for newcomers to contribute to the library. I realize that if you know TS, then technically it makes it easier to contribute and not mess things ups, but at the same time, people who don't know TS are now a bit less enthused to contribute. If we can ease that feeling by making the types as clean and as inferred as possible, I may be swayed.
  • I am by no means a TS-first developer, but I feel comfortable saying that if I can't understand a type signature or typing strategy in TS, I won't want to use it, nonetheless expose it to my users and expect them to understand it.
  • I have tried converting both React Query and React Table to TS a few times and get stuck pretty easily due to the dynamic nature of both libraries and their flexible APIs that, while being logically sound and well tested, are very very difficult to statically type.

With all that said, I am open to it. I can't give any guarantees that I won't smash my computer in a TS rage and put it off for another year... but I'm willing to try.

Thoughts?

@boschni
Copy link
Collaborator Author

boschni commented Jul 17, 2020

Hope it works out too let’s see ;) I think your requirements make sense and I actually agree to all of them.

It should definitely be a gradual process while keeping everything backwards compatible, including the current type definitions. This gives us time to work on the internal typings without affecting anyone. At some point the new types could be published as breaking change, or removed if it does turn out to be too difficult. Because transpilation is still done with Babel, and TypeScript is only used as advanced linter, it should be pretty easy to revert if needed. This setup at least gives us the possibility to try without much risk.

How would you like to proceed with this?

@tannerlinsley
Copy link
Collaborator

I think the best way forward would be to start "migrating" the types from the type definitions into the primitive-ish interfaces of the library. Queries, instances, caches, etc. Not really focus on typing all of the functions and what not yet, but just working on all of the different versions of the objects and interfaces, all with the primary goal of just not touching any of the original source code other than simply adding type references to the objects going in and out. Sure, there will be tons of errors at that point. But once it's done, that will be a good point to know if and how much the library actually needs to change logically to accomodate the types. If it's only a little, we'll make the changes and stay non-breaking :) If it's a lot, then we'll just have to discuss on how to go forward (complex types, a breaking rewrite, etc). But I think ingesting as much of the type definitions as possible and seeing what's left will be a good start.

@tannerlinsley
Copy link
Collaborator

Run into anything tricky yet?

@boschni
Copy link
Collaborator Author

boschni commented Jul 20, 2020

I managed to get almost everything typed while fixing some bugs along the way. The tricky parts are these:

  • Different configurations are merged and augmented at various stages, this makes is a bit unclear which configuration belongs to which stage.
  • Query accepts a single configuration object, but there are multiple type of configurations (regular/paginated/infinity). This makes it a bit hard to know which properties are available at what time.
  • In Query, the data property is T[] in infinite mode, but T in regular mode. This is a bit hard to type. Also the returned values are different in each mode.

A possible solution would be to create specific queries like Query, PaginatedQuery, InfiniteQuery and BaseQuery. These could encapsulate different behaviours instead of having them all in Query and would be line with the hooks.

What do you think of the MR? The types might not be 100% yet, but they could be refined along the way as they are not exposed yet.

@tannerlinsley
Copy link
Collaborator

Nothing jumped out at me on my initial pass through. Moving to classes was interesting... but I realize that they might be easier to type than object closures. I really dislike classes, but if that's a necessary change, then I think I can live with it.

@boschni
Copy link
Collaborator Author

boschni commented Jul 24, 2020

Is there any specific reason for this change? I have used this at quite a few places since most often then not, the params that you use in your queryFunction are the ones that would need to go in the queryKey.

It is mostly to keep the types simple for other contributors but also for library consumers who want to define the error type or create their own hooks. Personally I don't have a strong opinion on it though it is open for discussion. The impact on keeping this functionality can be seen here: boschni@0ff7972

@ayush987goyal
Copy link

Hi @boschni , I was wondering if we want to rename the types etc. in core which are prefixed with React (eg. ReactQueryConfig) to not have it? I am saying this because we are going to migrate core to a separate package and then consume it at multiple places (eg. react-query, vue-query etc.). If we have React specific types and exports in core, it would be confusing for other consumers. I suggest we remove those prefixes and have a generic names for them (eg. CoreQueryConfig). You might not have to worry about breaking current users because we are anyway targeting TS migration to v3. Thoughts? @tannerlinsley @boschni

@boschni
Copy link
Collaborator Author

boschni commented Jul 26, 2020

Hi @ayush987goyal , I think eventually the react-query package should still expose a ReactQueryConfig type because there might be configuration options specific to React (like for example the suspense, useErrorBoundary and maybe even mutations options). Other frameworks might have different ones.

When creating a separate QueryCache or QueryClient library, it would be nice if it was truly framework agnostic. Meaning it should be generic and flexible enough to support these kind of framework specific functionalities while not knowing about them, provide APIs for things like pagination and infinite queries, and it should be straightforward to use outside of any view layer.

The actual logic around suspense could then hopefully be implemented within react-query. What I mean is: does it make sense to separate these types now, even though we don’t really know yet how the new library types would look like? Maybe some of these things (like extracting any React specific functionality from the query cache) should be explored first in the current repo?

@boschni boschni force-pushed the feature/typescript branch from f46f87a to 20bbedf Compare July 29, 2020 20:43
@tannerlinsley tannerlinsley merged commit e23f934 into TanStack:master Jul 29, 2020
@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 2.5.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants