Typing support

[nickserv]

A lot of Redux users use TypeScript or Flow, and with Redux 4 out, I think it would be a good idea to provide optional types for this module. I would personally prefer to start by porting the codebase to TypeScript, but we could use separate definition files if desired. I may give this a shot but I want to wait for feedback and fix #28 first.

[nickserv]

@markerikson Do you want me to port the codebase to TypeScript? If not, I will add the type declarations to DefinitelyTyped instead of this package, and TypeScript users will have to install them separately.

[markerikson]

Go ahead and give it a shot if you'd like. Not my highest priority (I'm fine with starting to push this out to the public even if the type story isn't "right"), but if you'd like to tackle that aspect I can see it being useful.

[nickserv]

To be clear I mean actually replacing JS files with TS files using TS type syntax (though the syntax is back compatible with JS). Idk if it might affect contributions from others.

[neurosnap]

My recommendation would be to switch the project to typescript. Having built projects with javascript and retrofitting typedefs and starting with typescript, it's much easier to deal with the latter. I'd be happy to help. I already have createReducer typed for my own personal projects.

[markerikson]

I have no immediate complaints about doing so. But, I want to get this project usable and releasable ASAP. While I appreciate the potential values of static typing and that TS has grown a lot in popularity, I also have seen some painful bikeshedding with people tying themselves in knots over getting their types "perfect". If we can do this in TS relatively quickly, great! If it seems like switching to TS is holding us back from getting the project out the door, that would be a problem.

[nickserv]

@neurosnap Good point, I have found that typing is easier the earlier you start, whether they're external definitions (which only type the public API) or full source typings. Also, because TypeScript declarations override typings and can't be validated against against original source files directly, it's safest to port the sources to TypeScript (or something else like Flow that can be regenerated into TypeScript accurately). Anyway, would you mind posting the createReducer typings? I already started typing it but it would be useful to compare, and I could share my branch with you when it's more ready.

@markerikson I agree. My TypeScript experience is still kind of limited, though everything except selectorator should be relatively easy to type as far as I'm aware. I'll keep experimenting and we can leave it for later if it becomes too complex.

[neurosnap]

import createNextState from 'immer';
import { Action } from 'redux';

interface ActionsMap<S> {
  [key: string]: (state: S, action: Action) => S | void;
}

export default function createReducer<S>(
  initialState: S,
  actionsMap: ActionsMap<S>,
) {
  return (state: S = initialState, action: Action) => {
    return createNextState(<any>state, (draft: S) => {
      const caseReducer = actionsMap[action.type];

      if (caseReducer) {
        return caseReducer(draft, action);
      }

      return draft;
    });
  };
}

[nickserv]

I'm stuck on Selectorator types again and I'm starting to think this can't be as safe as Reselect in TypeScript. Basically TypeScript's tuple mapping inference works over spread parameters in TypeScript 3, but because Reselect does not use trailing spread args and Selectorator uses an Array argument, it is not supported for either package's declaration.

We have several options, I'd appreciate opinions/votes:

1. Replace Selectorator with Reselect. This is my preference because selector type safety is important and a lot of work has gone into the Reselect type declarations, but they're very complex and Selectorator's API is even more complex so I don't think this level of safety is practical to achieve with Selectorator. While this means less features for users, it means more stability for TypeScript projects.
2. Make Selectorator types less safe than Reselect types. Reselect currently maps selector types to the combiner type manually. Selectorator has a more complex API and it would be difficult to achieve this, so instead we can make the types a union and not order dependent, making them less valid.
3. Remove selector libraries. As I suggested in Remove re-exports #31, we may want to remove our dependency on Selectorator/Reselect entirely, in which case this isn't as much of an issue because TypeScript users won't get untyped package errors when installing redux-starter-kit and they can choose to use Reselect for the additional type safety.
4. Fix Selectorator definitions (see test file).
5. Avoid TypeScript support.

[neurosnap]

If we are not doing anything different from selectorator then to me it would make sense to reference it as an officially supported solution but to not include it inside this package's dependencies.

I would also say that in terms of maintenance, it will be annoying to update dependencies for something this package doesn't even use.

At the very least it should be a peerDependency, but I would argue that it isn't saving much effort for the end developer by re-exporting it.

[markerikson]

Except that part of the point of this package is to keep the end user from having to explicitly add commonly used pieces to their package.json. In this case, redux-thunk, reselect/selectorator, and redux-devtools-extension.

I see lots of complaints about having to add extra deps explicitly. One of my goals for this is to knock the most common obvious pieces off that list.

[neurosnap]

Gotcha, given this requirement, I would say that typescript is still extremely important in the ecosystem and a requirement for mass adoption at this point.

[awkannan]

Just providing my 2 cents here - I really like that Mark found selectorator. Reselect is great, but there is a slight learning curve, and it's easy for a beginner to get wrong. I also think that Types are important. I don't use TypeScript at work, but I have started using it in personal projects and it definitely feels like the ecosystem is headed in that direction.

The two almost feel at odds - selectorator has some magic, but makes it easier for a beginner user to write performant code easily. The API is more difficult to generate safe types for.

Would it be fair to punt on this problem? Ship it initially without the types around selectors and continue to work on it later?

[nickserv]

I've changed my mind. I think I can get my Selectorator types to the state where they may be useful for TS users, but because they will be less safe, I do not want Selectorator included with the package. I will try to release @types/selectorator, and I am in favor of merging #31 before publishing TS support. Alternatively, we can replace Selectorator with Reselect or give up on TypeScript support.

@neurosnap Agreed. While I think it's still possible for a new package to be successful without TS support (especially thanks to DefinitelyTyped), it's important to understand that many beginners rely on TypeScript's autocompletion to learn new packages, even if they aren't using TS files. Note that VSCode automatically learns autocompletion for packages used in JS files from their TS types.

@markerikson I disagree, I would argue this approach is against the design of npm and yarn packages. npm and Yarn are incapable of properly resolving dependencies without version conflicts when userland code directly uses a package that is an indirect dependency without depending on it directly. peerDependencies were created to solve this problem, and it is much easier for a user to just install some extra peerDependencies than have to deal with very unstable version conflicts as other packages update. Ultimately this is a problem with the design of npm/yarn and we should prefer a stable package over a workaround for the inconvenience. Note that the npx peerdeps command is a really convenient way to automatically install all peer dependencies of a package, and is safe to use.

@awkannan1 "Would it be fair to punt on this problem?" When a dependency package is re-exported (which as I mentioned above is bad practice anyway), the dependency's API is now considered part of the API of the top level package, so it needs to be typed if the top level package is typed. The best way to punt the problem is to remove selectorator from the package, mention both reselect and selectorator in the readme, and let the user install their choice based on what they're familiar with and if they use TypeScript.

[nickserv]

I'm getting close in #38. Haven't finished selectorator yet.

[denisw]

Second-round attempt at #73.

[markerikson]

Should be resolved by #73 .

[enjikaka]

Are types for flow planned? I see only TS got implemented.

[markerikson]

Nope. Flow is effectively dead - a recent /r/reactjs survey showed ~2% usage, while TS had ~50%.

If someone from the community wants to write Flow types, go ahead, but RTK is written in TS, the Redux core has been converted to TS on master, and there's no interest from us in touching Flow at all.

[enjikaka]

I get that, fair decision. :) I'm no fan, but sadly we're stuck with flow at work in the foreseeable future. I submitted a PR proposing us using Redux Toolkit, might not get through because of the typing we loose though.

If so I might have a look at converting the TS types to flow. Since flow was discussed earlier I just wanted to check if it was already in the works before heading down into the dreadful pain of making types for it myself. 😅

[markerikson]

Frankly, use of Flow is going to constrain your use of any library these days. While I don't have exact stats, my understanding is that flow-typed's list of covered libraries has always been a tiny fraction of DefinitelyTyped's support, and ditto for libs that ship Flow typings natively. In addition, the small size of the Flow community is going to minimize the amount of knowledge and support for those typings.

I'm not saying use of Flow is a bad thing, but it lost the competition, and anyone using it is going to have to deal with the lack of network effects accordingly.

[enjikaka]

Yeah, I'm aware of the situation. Using Flow is definitely a bad thing, and has always been.

Not only is the library support lacking like you point out, but it is also very resource heavy compared to TypeScript. While TypeScript is nearly instant and barely noticeable while doing its checks; flow takes minutes after changes to update - and makes the fans of any MacBook fly through the roof while doing so, at times even crashing mine completely so that it reboots. It's pure shit through and through; if you ask me. It was a bad decision to choose that over TypeScript to begin with, I did my best to stop it but without luck.

Migrating is not easy, but I hope we get there. :)

[nickserv]

FWIW if you do decide on migrating to TS, it may be easiest to start with the Babel loader with the Flow preset, and then use a TypeScript loader (ts-loader or awesome-typescript-loader) with compilerOptions: { "allowJs": true, "checkJs": true }. This hopefully should allow you to get some basic type checking against the TS declarations before migrating any code to TS.

[enjikaka]

@nickmccurdy Thanks for the tip!

[phryneas]

Also you might want to take a look at https://github.com/benjie/flow2typescript if you ever plan to convert.
It's being used to convert postgraphile to 100% TypeScript right now and seems to be working quite well.

[enjikaka]

I've ported createAction and createReducer the best I could now: flow-typed/flow-typed#3906