Port to TypeScript #3500
Comments
|
I'd like to see redux ported to TypeScript because TypeScript is the only way to ensure types are always in sync. Types that are always in sync prevent issues from being reported, improve the overall developer experience, and increase the overall potential for adoption of the library. The general benefits of types are that they improve the quality and productivity of the developer experience, even for non TypeScript developers. Types help catch bugs, enable better tooling, and enhance understanding. Types provide these benefits most effectively when they perfectly match the codebase. Leaving types to a separate community all but ensures a lag between library changes and the related type definition changes, making it an overall less effective approach to providing types and therefore lessening the overall value of the library. Regarding effort, because TypeScript is a superset of JavaScript the code base can be converted gradually. Gradual conversion means ongoing efforts are only minimally disrupted. Converting to TypeScript can also be done as a nonbreaking change, meaning it is also less effort for developers to upgrade to. Finally, TypeScript it's self has a vibrant community. Conversion to TypeScript may garner additional contributions to help with the effort. |
|
Relevant Twitter discussion thread: |
|
@jamestharpe : realistically, Redux is not going to be ported to TS any time soon. Tim and I don't have enough TS experience to do the porting or maintenance ourselves, and even if someone else does the work, that basically makes the code impossible for us to maintain. That's already happened over in Redux Starter Kit - it was ported by several users, and now I am effectively unable to make meaningful changes to the code myself. |
|
@markerikson Redux might be small enough to learn TypeScript 😊 That being said, I prefer types bundled with the package instead of DefinitelyTyped. The reason is simple. It's a sync issue. If the types are bundled with the package, they are usually updated with breaking changes and additions as opposed to after. I've helped moving the React Navigation types into the package for exactly that reason. |
|
Coming from a typescript fanboy, I don’t think redux should be ported to typescript especially if the maintainers don’t have a strong TS background. I personally think the typing should be removed from this library and moved to definitely typed. I think the community will do a great job maintaining them. Just look at react itself. Definitely typed manages those and they’re great. Given the popularity of this library, I don’t think there will be much lag time between versions even. |
|
I've got a pretty handle on our types at this point. There are some things I would like to add (more inferred types, for instance), but those are nice-to-haves, not things that make the types unusable or inaccurate. I do get the concerns around independent versioning, but I don't think there's any particular need to update them right now, so that's not really a practical concern. I've actually already tried this with redux-thunk and it's a complete rats nest. The linting alone was a giant hassle; other dependent packages hadn't been updated in a while and were out of compliance with the latest config. And there are a lot of things that rely on redux that are going to trip those same set of issues. It's a giant mess to unravel. |
|
Overall I think moving to DefinitelyTyped is the best option. Yes, TS generates more accurate types, but it’s too much work for Redux to migrate without a rewrite. We can improve the current state of typings by maintaining them separately. Bundling like we’re doing now is not recommended by Microsoft. |
|
@janhesters : I have learned TS. I just don't know TS well enough to be able to do stuff like this: Lines 375 to 384 in 63dda81 or this, from Redux Starter Kit: type CaseReducerActions<CR extends SliceCaseReducers<any, any>> = {
[T in keyof CR]: CR[T] extends (state: any) => any
? PayloadActionCreator<void>
: (CR[T] extends (state: any, action: PayloadAction<infer P>) => any
? PayloadActionCreator<P>
: CR[T] extends { prepare: PrepareAction<infer P> }
? PayloadActionCreator<P, string, CR[T]['prepare']>
: PayloadActionCreator<void>)
}
type NoInfer<T> = [T][T extends any ? 0 : never];
type SliceCaseReducersCheck<S, ACR> = {
[P in keyof ACR] : ACR[P] extends {
reducer(s:S, action?: { payload: infer O }): any
} ? {
prepare(...a:never[]): { payload: O }
} : {
}
} |
|
For libraries that are not written in TS I’ve noticed a worrying lag in types being added to definitely typed vs maintained in the repo. I’ve also seen more bugs and errors in the typings which lead to issues getting raised in the library even though these only relate to the types. Based on the latest JS surveys and extrapolating out I would expect that this year around 55% of JS devs will have used TS and be happy to use it again. At the point that the majority of JS devs are actually TS devs, every JS library needs to consider TS as a majority use-case. This means eventual death to any lib that does not have good typings available. But, I do appreciate that types for non-trivial libraries are a massive undertaking so I do see why the maintainers would want to discuss this. |
|
If the maintainers aren't TS users, I think maintaining types as part of release is problematic because:
|
|
Given the maintainer’s lack of interest in porting to TypeScript, I think DT is the way to go.
|
|
It seems like moving to DefinitelyTyped is preferred by most users and maintainers. @markerikson @timdorr How would you like to proceed? I’m thinking we could create a branch/PR that removes Redux types, then PR the Redux package types to DefinitelyTyped (may involve some extra code style and test work to meet their review criteria). Once that’s published, it should be safe to merge the removal of the declaration file as a major release (since it’s a breaking change for TypeScript users). |
|
FWIW, Daniel Rosenwasser from the TS team says they should be moved to DT: https://twitter.com/drosenwasser/status/1160628533166628864 I personally would be in favor of moving them to DT, but I know @timdorr has expressed interest in keeping them here. So, I'll leave it up to him to decide. |
|
All right, so I'll throw this out there just to bring it up: @timdorr , do we ever foresee the rest of the Redux libs getting rewritten in TS? I would assume no, but just want to get your thoughts on that for the record. |
|
I think the benefits of maintaining code and types together would outweigh the cost of migrating to TypeScript for smaller packages like redux-thunk and reselect (which has extremely complex types that are harder to test because they're separate from the source code). It still might backfire if the maintainers don't have TypeScript experience, but they're simple enough that they shouldn't go very out of sync. |
|
My concern with switching anything to TS is approachability. Even after transpilation and bundling, our code is still quite readable. And given the size of the library and its importance in the ecosystem, that's something I do not ever want to compromise on. In addition, our types are also pretty well documented and quite functional at this point. Again, we really only have improvements to make to them, not fixes. So other than the decorum of moving them elsewhere, I don't see that changing how much they're developed or often they're released. For redux at least, I'm pretty much fully against this. It makes a lot more sense for things like redux-thunk (which I had huge problems when I last tried it...), but not for this small, central library. |
For clarification on my above comment, if there is no real problem, then I wouldn't fix it. My vote is to do what @timdorr wants to do. |
|
|
|
Just making a copy of them? Sure, but I'm not sure if that would create problems for the other type packages that depend on them. |
|
No, I mean deleting them from Redux as a breaking change and major release. TypeScript only recommends publishing types when the package is written in TypeScript. |
|
No, I said I'm fully against that. I want to keep them here. |
|
Can we discuss why? This is not recommended by the TypeScript documentation and I think it’s been causing versioning and maintenance issues with Redux. A Microsoft employee on the core TypeScript team has specifically suggested we move to DefinitelyTyped. Removing types is the simplest way to solve the problem without porting the code to TypeScript (which I don’t think is a good idea in this situation). |
|
Keep typing here will save us version problem and easy to change. |
|
I've already stated my reasons above. Honestly, DT is just a maze of tooling full of landmines and gotchas. It's a half-gig of git data with 6000+ types and nearly 3000 open issues. It's extremely hard to get into, and the tooling that's in place doesn't make it particularly easy to get started. When I tried to move in the redux-thunk types (one project where I do support the move), I couldn't figure it out. All the docs were unhelpful and the tooling gave no reasoning for why unrelated types were failing. Again, one thing I really like about Redux and is very important to its community, is its approachability as a code base. DT is the opposite of this, IMO. Don't get me wrong, I think it's great to have a central clearinghouse for type definitions for projects unaware of TS. But the nature of the Redux community runs counter to that. I think the perception of TS being a maintenance issue might be due to the past few years where I didn't know any TS and relied heavily on the community. I'm at the level where I may not be the best TS coder, but I think I've gotten pretty good. Certainly good enough where I can offer more meaningful review of type PRs. For all intents and purposes, Redux is "done" right now. We don't have plans for another major (including the types), so the types can be the driver on moving the version numbers forward. That's totally fine in my mind. It doesn't create a maintenance problem because the actual code we distribute stays the same and we can easily check that it's not changing from release to release (I always run One thing that might be an interesting experiment would be a parallel implementation of the code in TypeScript. Basically, move the types out of the definition files to the code itself, side-by-side with the plain JS implementation. That's definitely a more complex setup, but it may make the type def generation easier and less like the fancy examples that Mark posted above. Again, this is made feasible by the code being "done" and unlikely to receive major changes in the future. Either that or we do actually rewrite into TS and have some pre-commit hooks to transpile down to plain JS and store that in the repo. Let me do a Twitter poll for science. |
|
TypeScript is designed to keep separate types in DefinitelyTyped, it's still a better option even if it's more difficult to use at first. DefinitelyTyped will get easier to contribute to over time as it matures, and it already has some very useful tooling that makes this kind of type work easier. Keeping types here involves extra maintenance churn even if we don't care about version numbers, and prevents us from using the latest tools and configuration without setting them up manually. Also, I don't think we should have a second implementation in TypeScript, unless it has breaking changes to the API. If we can manage to rewrite Redux with full API compatibility (and our tests should make that easy to determine), we can just replace the JS implementation with the TS one, and it will still be fully usable by JS users. TypeScript packages are actually published as plain JS, with the TS declaration files automatically generated, so they work well with both languages. @timdorr Before I forget, how do you feel about handling this for reselect, redux-devtools, and redux-thunk? Would you have the same decision as here, or would it be useful for me to open similar issues for discussion specific to those repositories? |
|
note that export type DeepPartial<T> = {
[K in keyof T]?: T[K] extends object ? DeepPartial<T[K]> : T[K]
}Any objections? |
|
Yeah, I was waiting for the next major to drop it, since we export it and others may be using it. It can go for 5.0. |
|
Hey @timdorr and @markerikson, please let me know if I can help at all with this TypeScript conversion. I'm super comfortable with TypeScript and I don't want you guys to struggle. I also realize you're using this as a sort of learning opportunity, so best of luck! I know you already know the TS basics, so I want to recommend that the most important features of TS you need to learn are generics and conditional types. Master these concepts and you will be a TypeScript god. Fortunately, the TS documentation is super good. |
|
And we are now converted to TS! 💃 |
|
This effort seems to have stalled a bit. Per discussion with Tim, it sounds like the main conversion work is done, and now it's a question of what improvements can be made to the TS types (including potentially breaking changes). What potential types improvements are we thinking of? If the I know there's also some discussion about Anything else? |
|
Does anyone know what |
|
yeah.. you removed it :) I strongly suggest reverting this commit. |
|
I feel I was a bit hasty in tossing off a quick reply. Let me elaborate. I used the tests in There are, of course, probably other ways to do this, but since the infrastructure is already here, I recommend using it. |
|
one last comment - the only problem in import { checkDirectory } from 'typings-tester'
describe('TypeScript definitions', function() {
it('should compile against index.d.ts', () => {
checkDirectory(__dirname + '/typescript')
})
})and should be something like: import { checkDirectory } from 'typings-tester'
describe('TypeScript definitions', function() {
it('should verify types fail when appropriate as well as working with normal usage', () => {
checkDirectory(__dirname + '/typescript')
})
}) |
|
Use dtslint instead, it's officially supported by Microsoft. |
|
nice! following the link at dtslint which says "if you only need ExpectType and ExpectError" we get https://github.com/SamVerschueren/tsd What do you think about using that @nickmccurdy ? |
|
Not sure, I haven’t heard of it, but sounds worth trying |
|
Use neither.
https://github.com/microsoft/dtslint#setup The basic point is that you don't have to "test the types" if the types were generated, as you can trust they are accurate. This simplifies libraries that are written in TypeScript. Please, don't reintroduce these types of tests. |
|
Ah, I missed that. Have we made sure those test cases are tested in our new TypeScript tests? |
|
@jedmao : I'd disagree with that. We're using some additional types tests over in Redux Starter Kit to make sure the types actually behave the way we want, and they've found several important problems. Example: |
|
Again, the tests check for things that are not otherwise possible to test for. Simply assuming they work without testing that they actually fail on invalid cases is risky for a library |
|
@cellog Are you suggesting that we should assert both type errors and runtime errors using dynamic tests? There are some scenarios where types may be incorrect even if the runtime behavior is correct. I think @markerikson has a good point that we should still keep some sort of type tests for invalid cases. |
|
That's a good example, but I wouldn't apply it wholesale across the entire library. Just where it's relevant. Looks like tsd is the better choice, as it's limited in scope and doesn't use the antiquated TSLint. |
Do you want to request a feature or report a bug?
No
What is the current behavior?
Redux publishes a manually created index.d.ts file. This also affects reselect, redux-devtools, and redux-thunk.
If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar.
Explanation of why this isn't recommended: https://www.typescriptlang.org/docs/handbook/declaration-files/publishing.html
To summarize, type declarations should either be generated from TypeScript source code or shared in DefinitelyTyped, which allows contributors to edit the types without having to edit the original package. This can also help alleviate maintenance and versioning issues regarding Redux users that write their code in TypeScript. Many issues about TypeScript compatibility could be moved to the dedicated DefinitelyTyped community if the types were separated, and the types could have breaking changes released without interfering with non-TypeScript users.
What is the expected behavior?
Redux either removes its type declaration (should be a breaking change) and moves them to DefinitelyTyped (recommended) or is ported to TypeScript (could be difficult and requires additional maintenance, but makes the types more accurate).
Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?
3.4.0+ (all versions that publish index.d.ts)
The text was updated successfully, but these errors were encountered: