* refactor actionCreator typings from `type` to `interface` #273

phryneas · 2019-12-06T21:26:43Z

This has been in my head since @dragomirtitian made me aware that TypeScript error messages (and tooltips) are not very consistent for types, but they are for interfaces.

So the current behaviour is that the tooltip for createAction<number, 'test'>('test') would read

WithTypeProperty<WithMatch<(<PT extends number>(payload: PT) => WithPayload<PT, Action<"test">>), "test", number, never, never>, "test">

After this PR, the same return type would read

ActionCreatorWithPayload<number, "test">

which is WAY more descriptive and should help our users.

While doing that, I also noticed that the order of operation for the different cases of the PayloadActionCreator type was wrong and skipping some cases. I also added a type test for that.

This is not finished yet (I have to adjust the createSlice typings and while I'm at it shorten those a bit - I've learned a new trick or two), but I thought I'd make transparent that I'm currently working on this by already opening a PR.

PS: I'm trying to stay as much to the current interfaces as possible, so this should not bring any breaking changes with it - I think by now I haven't changed anything external-facing that wasn't a bug before. The type-tests are a blessing for that 😄

PPS: ah, the joys of different behaviour in different TS versions... well, at least the CI makes me aware of it

netlify · 2019-12-06T21:27:31Z

Deploy preview for redux-starter-kit-docs ready!

Built with commit bc2f68e

https://deploy-preview-273--redux-starter-kit-docs.netlify.com

codesandbox-ci · 2019-12-06T21:29:07Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bc2f68e:

Sandbox	Source
zen-lederberg-cjnw9	Configuration
runtime-brook-2iuqs	Configuration
rsk-github-issues-example	Configuration

dragomirtitian · 2019-12-07T06:59:11Z

@phryneas Glad to see someone is putting the interface trick to good use :)

so that they keep their name in TS errors & tooltips * fix order of operation for PayloadActionCreator

phryneas · 2019-12-07T16:41:25Z

Okay, so I've done some more things:

added additional type-tests for useReducers
I updated tsdx to 0.11
I re-added @microsoft/api-extractor to do the .d.ts rollup. This also generates an API report that should be committed with the code - essentially a normalized version of all types in one file. This allows us to see changes in the external API quickly in PRs. If someone does not commit the newest version of this file, CI will fail.
This gives me some security that I don't add breaking changes without noticing it.

A diff between two reports looks like this:
report

I'll annotate it so that you can see what changed without going through all the other files :)

phryneas

Some explanations what I did here :)

phryneas · 2019-12-07T16:42:32Z

etc/redux-toolkit.api.md

+// Warning: (ae-missing-release-tag) "ActionCreatorWithNonInferrablePayload" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
+//
+// @public (undocumented)
+export interface ActionCreatorWithNonInferrablePayload<T extends string = string> extends BaseActionCreator<unknown, T> {


added a new interface

phryneas · 2019-12-07T16:42:53Z

etc/redux-toolkit.api.md

 // Warning: (ae-missing-release-tag) "ActionCreatorWithOptionalPayload" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public (undocumented)
-export type ActionCreatorWithOptionalPayload<P, T extends string = string> = WithTypePropertyAndMatch<{
+export interface ActionCreatorWithOptionalPayload<P, T extends string = string> extends BaseActionCreator<P, T> {


changed type to interface, external signature stays the same

phryneas · 2019-12-07T16:43:05Z

etc/redux-toolkit.api.md


 // Warning: (ae-missing-release-tag) "ActionCreatorWithoutPayload" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public (undocumented)
-export type ActionCreatorWithoutPayload<T extends string = string> = WithTypePropertyAndMatch<() => PayloadAction<undefined, T>, T, undefined>;
+export interface ActionCreatorWithoutPayload<T extends string = string> extends BaseActionCreator<undefined, T> {


changed type to interface, external signature stays the same

phryneas · 2019-12-07T16:43:33Z

etc/redux-toolkit.api.md

 // Warning: (ae-missing-release-tag) "ActionCreatorWithPayload" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public (undocumented)
-export type ActionCreatorWithPayload<P, T extends string = string> = WithTypePropertyAndMatch<IsUnknownOrNonInferrable<P, <PT extends unknown>(payload: PT) => PayloadAction<PT, T>, <PT extends P>(payload: PT) => PayloadAction<PT, T>>, T, P>;
+export interface ActionCreatorWithPayload<P, T extends string = string> extends BaseActionCreator<P, T> {


changed type to interface, external signature stays the same

phryneas · 2019-12-07T16:45:36Z

etc/redux-toolkit.api.md

 // Warning: (ae-missing-release-tag) "ActionCreatorWithPreparedPayload" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public (undocumented)
-export type ActionCreatorWithPreparedPayload<PA extends PrepareAction<any> | void, T extends string = string> = PA extends PrepareAction<infer P> ? WithTypePropertyAndMatch<(...args: Parameters<PA>) => PayloadAction<P, T, MetaOrNever<PA>, ErrorOrNever<PA>>, T, P, MetaOrNever<PA>, ErrorOrNever<PA>> : void;
+export interface ActionCreatorWithPreparedPayload<Args extends unknown[], P, T extends string = string, E = never, M = never> extends BaseActionCreator<P, T, M, E> {


Renamed type ActionCreatorWithPreparedPayload to _ActionCreatorWithPreparedPayload. This type had a signature that made it almost unreadable for external use, but very handy for internal use.

Created new interface with the old name ActionCreatorWithPreparedPayload, now with a more readable signature. The original renamed type essentially just redirects to this new type.

@markerikson I've thought about this one, and given it's the only breaking typechange, I'd rather like to avoid that.
An alternative would be to keep the type ActionCreatorWithPreparedPayload with the existing signature instead of renaming it and introducing a new interface to be exposed to the user.
But I'd need a new name for that and I'm not 100% sure what would be fitting. Something along the lines of PreparedActionCreator or EnhancedActionCreator. What do you think?

So this is where we get into the question of "what really is a breaking change, anyway?".

Is it a literal change to an API definition that is different than how the old one behaved? Is it something that causes runtime errors? Is it something that causes a compilation failure?

One of the downsides I've observed with TS is that almost any change can result in a compile error, even for stuff that's only intended to fix some bugs. Do those count as "breaking changes"?

Given that, I am inclined to treat stuff like this as a patch or a minor release, rather than a semver major. We've identified an issue with the types that doesn't actually affect the rest of the functionality, and we're trying to resolve that issue.

If you're worried about folks relying on that particular type, a quick search on Github turns up exactly 3 hits: 2 in RTK, and 1 where someone copy-pasted some RTK code. So I don't expect this will suddenly break people.

Yup, that stuff gets pretty philosophical pretty fast.

In this case, I guess it's okay since the original type was very unlikely to be used by anyone (it was very unwieldy and I don't see a reason why someone would want to manually specify it anyways), as your github search confirms.

In general, I guess we'll do good to keep those type APIs as stable as possible, but that's what we've got these reports for, going forward from now. I guess people are somehow used to types breaking, but we'd probably do good to handle type-breaking changes as at least minor releases - I guess it should be possible to apply patch-level upgrades without second thought or consequence.

Yeah, I'm on board with that. So, this would end up as 1.2.0.

I certainly don't want to go randomly breaking types, but I do feel that types changes should be treated with a bit more leeway in semver terms.

Maybe jaredpalmer/tsdx#365 will lead to something soon, then we could put the 1.2.0 out as a "types & tree-shaking" update.
Nice side-effect of this is that we aren't bound by tsdx 0.10 any more.

phryneas · 2019-12-07T16:47:03Z

etc/redux-toolkit.api.md

 // Warning: (ae-missing-release-tag) "PayloadAction" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public
-export type PayloadAction<P = void, T extends string = string, M = never, E = never> = WithOptional<M, E, WithPayload<P, Action<T>>>;
+export type PayloadAction<P = void, T extends string = string, M = never, E = never> = {


This has to stay a type, as interfaces cannot include conditional logic (like adding and removing the property meta depending on the type value of M).

But I inlined the logic, so that TS resolves it into one readable type as soon as type parameters are known.

phryneas · 2019-12-07T16:47:43Z

etc/redux-toolkit.api.md

 // Warning: (ae-missing-release-tag) "PayloadActionCreator" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal)
 //
 // @public
-export type PayloadActionCreator<P = void, T extends string = string, PA extends PrepareAction<P> | void = void> = IfPrepareActionMethodProvided<PA, ActionCreatorWithPreparedPayload<PA, T>, IfMaybeUndefined<P, ActionCreatorWithOptionalPayload<P, T>, IfVoid<P, ActionCreatorWithoutPayload<T>, ActionCreatorWithPayload<P, T>>>>;
+export type PayloadActionCreator<P = void, T extends string = string, PA extends PrepareAction<P> | void = void> = IfPrepareActionMethodProvided<PA, _ActionCreatorWithPreparedPayload<PA, T>, IsUnknownOrNonInferrable<P, ActionCreatorWithNonInferrablePayload<T>, IfVoid<P, ActionCreatorWithoutPayload<T>, IfMaybeUndefined<P, ActionCreatorWithOptionalPayload<P, T>, ActionCreatorWithPayload<P, T>>>>>;


The order of operation for that type was changed to fix some bugs.

phryneas · 2019-12-07T16:50:11Z

So, everything is running with all versions >= 3.3 again - from my side this is ready for you to take a look :)

Andarist · 2019-12-08T22:16:57Z

@phryneas maybe you would consider writing a blog post on writing stuff like this? I would love to learn more about TS craziness and truth to be told couldn't get any good resources, the knowledge is mostly spread across various SO answers.

phryneas · 2019-12-08T23:26:57Z

@phryneas maybe you would consider writing a blog post on writing stuff like this? I would love to learn more about TS craziness and truth to be told couldn't get any good resources, the knowledge is mostly spread across various SO answers.

@Andarist I intend to make a talk out of this ("Taming the ternary"), and test it at a meetup early next year. Once I have that slide deck I guess I'll either make a blog post or a video out of it. But realisticly, if I really wanted to cover everything I've learned in the last half year of writing types for RTK, it'd have to be a full-day workshop ;)

kevin940726 · 2019-12-13T09:25:32Z

This has been in my head since @dragomirtitian made me aware that TypeScript error messages (and tooltips) are not very consistent for types, but they are for interfaces.

@phryneas Sorry for being off-topic, but do you have any reference links for this? I would love to learn more about it!

phryneas · 2019-12-13T10:09:51Z

This has been in my head since @dragomirtitian made me aware that TypeScript error messages (and tooltips) are not very consistent for types, but they are for interfaces.

@phryneas Sorry for being off-topic, but do you have any reference links for this? I would love to learn more about it!

Unfortunatley not - he wanted to talk about it in this talk (recommended!) but ran out of time, so I just chatted a bit with him about it.
But essentially: TS seems to stop at "interface borders" when it displays error messages & tooltips, but drills eagerly into types, leading to readable error messages when using interfaces and often unreadable ones when using types.

kevin940726 · 2019-12-13T10:15:00Z

@phryneas Cool! This is super interesting, I always thought that it’s just about preference. Thanks a lot!

phryneas · 2019-12-13T10:27:05Z

There are some differences - in general, Types are more powerful than Interfaces as they can contain more complex maps and conditionals.
Interfaces on the other hand can do things that types couldn't, for a long time: interfaces are evaluated lazily, so they can be self-recursive. As types are eager by default, types could not be recursive until a patch in 3.7 made them lazy under some circumstances. I'm sure there's more to it, but that's most of what I know :)

markerikson · 2019-12-14T03:43:57Z

Don't think I have any complaints about the TS aspects. But, looks like we've got a conflict with the serializable middleware. Can you clean that up?

phryneas · 2019-12-14T11:37:11Z

Okay, it's mergable again. LGTM?

phryneas added 3 commits December 7, 2019 15:37

upgrade tsdx, re-introduce api-extractor

a8eabeb

* refactor actionCreator typings from type to interface

a1533f8

so that they keep their name in TS errors & tooltips * fix order of operation for PayloadActionCreator

fix for TS 3.3

64a40f8

phryneas force-pushed the typings-refactor branch from 6dd6fba to 9868ffd Compare December 7, 2019 16:18

minor tweaks, add test for createSlice

8517a47

phryneas force-pushed the typings-refactor branch 3 times, most recently from 260907f to 3f5edc2 Compare December 7, 2019 16:34

phryneas commented Dec 7, 2019

View reviewed changes

phryneas marked this pull request as ready for review December 7, 2019 16:49

add updated api report

bc2f68e

phryneas force-pushed the typings-refactor branch from 3f5edc2 to bc2f68e Compare December 14, 2019 11:29

markerikson merged commit aed0e4b into reduxjs:master Dec 14, 2019

phryneas mentioned this pull request Dec 21, 2019

exportable case reducers type, bugfix, documentation #290

Merged

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

* refactor actionCreator typings from `type` to `interface` #273

* refactor actionCreator typings from `type` to `interface` #273

phryneas commented Dec 6, 2019 •

edited

Loading

netlify bot commented Dec 6, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 6, 2019 •

edited

Loading

dragomirtitian commented Dec 7, 2019

phryneas commented Dec 7, 2019 •

edited

Loading

phryneas left a comment

phryneas Dec 7, 2019

phryneas Dec 7, 2019

phryneas Dec 7, 2019

phryneas Dec 7, 2019

phryneas Dec 7, 2019

phryneas Dec 8, 2019

markerikson Dec 8, 2019

phryneas Dec 8, 2019

markerikson Dec 8, 2019

phryneas Dec 8, 2019

phryneas Dec 7, 2019

phryneas Dec 7, 2019

phryneas commented Dec 7, 2019

Andarist commented Dec 8, 2019

phryneas commented Dec 8, 2019 •

edited

Loading

kevin940726 commented Dec 13, 2019

phryneas commented Dec 13, 2019

kevin940726 commented Dec 13, 2019

phryneas commented Dec 13, 2019

markerikson commented Dec 14, 2019

phryneas commented Dec 14, 2019

* refactor actionCreator typings from type to interface #273

* refactor actionCreator typings from type to interface #273

Conversation

phryneas commented Dec 6, 2019 • edited Loading

netlify bot commented Dec 6, 2019 • edited Loading

codesandbox-ci bot commented Dec 6, 2019 • edited Loading

dragomirtitian commented Dec 7, 2019

phryneas commented Dec 7, 2019 • edited Loading

phryneas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phryneas commented Dec 7, 2019

Andarist commented Dec 8, 2019

phryneas commented Dec 8, 2019 • edited Loading

kevin940726 commented Dec 13, 2019

phryneas commented Dec 13, 2019

kevin940726 commented Dec 13, 2019

phryneas commented Dec 13, 2019

markerikson commented Dec 14, 2019

phryneas commented Dec 14, 2019

* refactor actionCreator typings from `type` to `interface` #273

* refactor actionCreator typings from `type` to `interface` #273

phryneas commented Dec 6, 2019 •

edited

Loading

netlify bot commented Dec 6, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 6, 2019 •

edited

Loading

phryneas commented Dec 7, 2019 •

edited

Loading

phryneas commented Dec 8, 2019 •

edited

Loading