aspnet / JavaScriptServices

[Archived] This repository has been archived
Apache License 2.0
3.03k stars 518 forks source link

Consider removing redux-typed #387

Closed SteveSandersonMS closed 7 years ago

SteveSandersonMS commented 8 years ago

@emzero and others have stated a preference for removing the redux-typed dependency from ReactReduxSpa and instead only making use of the native type information provided by @types/react-redux etc.

You might be wondering why redux-typed was created, and why it's only (apparently) used in these project templates, not by the wider Redux community. redux-typed was created about a year ago, because:

  1. At that time, Redux didn't natively offer any TypeScript type hints
  2. At that time, Redux's APIs were seriously mismatched with the way TypeScript handles strong typing. In particular, Redux largely expected developers to construct props for components by merging together various other objects at runtime, e.g., like this. Because of this runtime-merging approach, there was no actual statically-determined type information that would give good completion hints, e.g., on this.props inside a component. As far as I can tell, the Redux community wasn't widely using TypeScript at that time.

So, redux-typed addressed this by (1) obviously providing TypeScript type hints, and (2) by an admittedly non-obvious but quite effective use of TypeScript's inference features and typeof operators to allow type info to flow directly from your stores/reducers/etc via the provide mapping into dynamically-constructed component-props types. It manages to create implicit types that combine all the properties from your stores/reducers/etc, in the same way that will happen at runtime.

However, I haven't personally been promoting the use of redux-typed via all the usual marketing channels (blog posts, conference talks, etc.), and not surprisingly, the library hasn't become hugely widespread without that sort of promotion. Very few people really know what it's doing or what benefits it offers.

Now as of October 2016, partly the original issues have subsided. For (1), there's now @types/react-redux etc., which covers most of the basics. But for (2), as far as I can tell, things have only improved slightly - examples like this suggest you still have to manually create TypeScript interfaces that duplicate the type information from your stores/actions/etc.

What we need to decide now is:

  1. Can we remove redux-typed? It would be great if we could, because like @emzero and others have pointed out, it's pretty non-standard in the Redux community, and may be a barrier for people trying to make sense of code they see elsewhere. Even if redux-typed still has benefits, it's not clear that they are so essential it's worth the cost of having something unusual in the templates.
  2. Or, do we decide that redux-typed is so much better than the alternative that it has to stay? This approach is only really viable if some people step up to own its maintentance and promotion within the Redux community. I don't expect to have capacity to do that myself.

If we go for option 1, then is somebody (@emzero?) willing to propose a PR that removes it, and updates the ReactReduxSpa template to contain the same functionality but without that dependency? I know the code will end up being more verbose because it will probably become necessary to specify things to TypeScript that redux-typed would have done automatically. But can the code still be straightforward enough for newcomers to work with?

empz commented 8 years ago

Yes, I'm willing to propose a PR removing redux-typed. In fact, today I was planning on doing that in my own project that's based on the ReactRedux template. So, just let me know what you guys decide and I'll happily submit a PR.

SteveSandersonMS commented 8 years ago

What we decide will depend a lot on how much better or worse the ReactReduxSpa code looks with the changes. So if you don't mind putting together what you think is the cleanest and simplest way to do it, we can definitely assess how desirable it would be to switch!

MrCrimp commented 8 years ago

I'm still learning TypeScript as I go, but for what it's worth I find redux-typed quite elegant. But the redux connection pattern gets a bit intens some places, I find myself writing code that I feel a strong need to simplify using convention or other clever patterns. Me and a colleague were struggling with getting the .withExternalProps to work as well and created intersected interface instead, there's probably a more elegant solution.

I could see myself creating some convention based solution to this in the future with a component name or displayName property that is used to resolve from the store, actions and connecting reducers, instead of this example where I add react-intl to the mix. Any suggestions to improve this are welcome.

const provider = provide(
  (state: ApplicationState) => state.app,
  shellActions.actionCreators
);

type mappedProps = typeof provider.allProps;
export interface SpecialProps {
  body: React.ReactElement<any>;
}
type LayoutProps = mappedProps & SpecialProps & DefaultStateProps;

const intl = injectIntl(LayoutComponent);
const container = provider.connect(intl);
export { container as Layout};
SteveSandersonMS commented 7 years ago

@emzero Are you still planning to propose an alternative to redux-typed?

SteveSandersonMS commented 7 years ago

Unless anyone objects, I'll close this.

It looks like we will continue with redux-typed unless anyone has a specific proposal for how to replace it!

MrCrimp commented 7 years ago

I'm convinced of deprecating. Very nice use of plain redux starting with this commit! https://github.com/aspnet/JavaScriptServices/commit/9b048c54d4cae1ab63d76889c7c78a27a715a603

SteveSandersonMS commented 7 years ago

OK, closed :)