Gutenboarding: data-store reducer function signatures don't need to list which actions they handle
#38932
Conversation
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~26828 bytes removed 📉 [gzipped]) Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~639276 bytes removed 📉 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~11507 bytes removed 📉 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~12545 bytes added 📈 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Moment.js Locales (~18456 bytes added 📈 [gzipped]) Locale data for moment.js. Unless you are upgrading the moment.js library, changes in these chunks are suspicious. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
p-jackson
changed the title
data-store reducer function signatures don't list which actions they handle
p-jackson
force-pushed
the
update/data-store-action-types
branch
from
fc991c6 to
fd10df9
Compare
p-jackson
force-pushed
the
update/data-store-action-types
branch
from
fd10df9 to
a86aad8
Compare
p-jackson
changed the title
data-store reducer function signatures don't list which actions they handledata-store reducer function signatures don't list which actions they handle
p-jackson
changed the title
data-store reducer function signatures don't list which actions they handledata-store reducer function signatures don't need to list which actions they handle
p-jackson
added
the
[Status] Needs Review
I like the motivation behind this PR, and think it makes 100% sense. I don't feel confident reviewing it from a TS perspective yet, but it doesn't break the frontend and I can't see any compilation errors |
|
Thanks @ramonjd, my main worry was it didn't make sense. I was struggling to word my rationale 😅 Obviously hoping for some 👀 from @Automattic/luna, but maybe I'll throw a @Automattic/type-review in for good measure. |
|
Nice work @p-jackson, I also don't feel confident reviewing it from a TS perspective, but for me this also greatly improves the readability of the reducers as that top block of passing in generics to In practical terms, I think it also makes sense because won't each of the reducers be called with each of the actions, whether or not their type is specified in the reducer's signature? So I think this change feels a bit more accurate to the behaviour of reducers.
I'm quite partial to function definitions and function expressions when functions are declared at the top-level, so I like this, too! |
| type DomainSuggestion = import('@automattic/data-stores').DomainSuggestions.DomainSuggestion; | ||
| type Template = import('@automattic/data-stores').VerticalsTemplates.Template; | ||
|
|
||
| function domain( state: DomainSuggestion | undefined = undefined, action: OnboardAction ) { |
There was a problem hiding this comment.
Setting the state's default value to undefined is redundant.
Also, using the Reducer<> type has some benefits, as it adds # | undefined automatically to the state type:
const domain: Reducer< DomainSuggestion, OnboardAction > = ( state, action ) => {
...
}I continue to be very confused about the OnboardAction type. I believe that the action type shouldn't be constrained in any way beyond requiring an object with type field. And that's what the import { Action } from 'redux' type specifies.
Redux will call the reducer with an init action like this one:
{
type: "@@redux/INITj.3.e.w.g"
}This action doesn't pass typecheck for OnboardingAction, and yet the reducer is guaranteed to be called with it and must react to it correctly.
On the other hand, I see exactly this kind of constraining the action type in the official docs, too:
https://redux.js.org/recipes/usage-with-typescript/#type-checking-reducers
Is it a bug, or am I just clueless about something?
There was a problem hiding this comment.
The Reducer type is very strange and I've wondered about this myself. We claim that a Reducer handles a specific action type, but as you say it must handle all actions.
I'd speculate it's a tradeoff the authors decided to make because by convention —you can't use Redux if you don't understand the if ( action.type === 'MY_ACTION' ) { return 'new state' } else { return state } pattern—, reducers gracefully handle all action types.
If we expect this by convention and lie a little bit to the type system, we get better information in our reducer functions. A correctly typed reducer function that wants to deal with a specific action but has to deal with actions of { type: any } ends up being painful to work with. Consider that we can't even get a discriminated union because the basic action type { type: any } also satisfies a fully typed { type: 'MY_ACTION', foo: string, bar: number, baz: boolean }, so in our if ( action.type === 'MY_ACTION' ) { … } branch, we still don't know whether which type we have!
I think the following illustrates it:
import { Reducer } from 'redux'
interface ReducerCorrect<S, A> {
// Note the `A | A_` here, our type effectively opens up to `{ type: any }` 😞
<A_ extends { type: any; }>(state: S | undefined, action: A | A_): S;
}
type Action = { type: 'foo', bar: { baz: { quux: number } } }
// This is unsafe, but the experience is better
const reducerRedux: Reducer<number, Action> = (state = 0, action) => {
// Whoa, that's going to error at runtime for sure!
// Of course, anyone experienced with Redux won't write this…
return action.bar.baz.quux;
}
// This is safe, but is a pain
const reducerCorrect: ReducerCorrect<number, Action> = (state = 0, action) => {
if (action.type === 'MY_ACTION') {
return action.bar.baz.quux; // We can't know this -- type error! 😭
}
return state;
}There was a problem hiding this comment.
My thoughts about typing tradeoffs echo @sirreal's. The global nature of Redux actions almost makes them untypable. Elm's pattern of using nested actions is a way of making all the types perfect, but global events are pretty handy :)
Setting the state's default value to undefined is redundant.
Good point, I'll clean that up while I'm here.
using the Reducer<> type has some benefits, as it adds # | undefined automatically to the state type
I actually like that not all state types are # | undefined. Yes the onboard store has a lot of optional pieces of state so it'd be a convenience here. But the domainSuggestion state is never undefined and I feel like a lot of reducers work this way e.g. a lot of reducers are just a single value type and have a default.
There was a problem hiding this comment.
🤔After considering this a bit more and seeing some other context (p1579980641021900-slack-typescript), I'd change my speculative stance 🙂
The second type argument to Reducer is fine if left to the default. But, if it becomes the union of the type of all actions in your application, it should give excellent information and type safety.
There was a problem hiding this comment.
Setting the
state's default value toundefinedis redundant.
I've pushed a fix for this.
The second type argument to
Reduceris fine if left to the default. But, if it becomes the union of the type of all actions in your application, it should give excellent information and type safety.
@sirreal Just to clarify, does that mean you're happy with the changes here since it allows reducers to accept any of the actions for a given store? I thought that was your original (speculative) stance.
There was a problem hiding this comment.
does that mean you're happy with the changes […]
I've been considering these changes and playing with the types but haven't found the time to leave a full review. I'd prefer to leave it for a while.
There was a problem hiding this comment.
It seems that the TypeScript types for Redux reducers are buggy. When I created a toy example of a reducer that accepts only a constrained set of actions, it indeed fails to compile valid dispatches of unknown actions:
function reducer( state, action: HandledActionTypes ) {
...
}
reducer( undefined, { type: '@@INIT' } );
createStore( reducer ).dispatch( { type: '@@INIT' } );Both these statements fail to compile and compiler reports an error:
Type '"@@INIT"' is not assignable to type '"ACTION_A" | "ACTION_B"'
The Redux repo already has a similar issue: reduxjs/redux#3580
It describes another funny consequence of the constrained action type: one can write a reducer that fails to handle unknown actions:
function reducer( state: MyState = initState, action: MyActions ): MyState {
switch ( action.type ) {
case MY_ACTION_A:
return { ...state, ... };
case MY_ACTION_B:
return { ...state, ... };
}
}The switch statement exhausts all possible values of action.type for the MyActions type, so TypeScript doesn't mind that it returns undefined for all other action.types. It thinks it can never happen 🙂 But such a reducer breaks the Redux contract, and also fails to always return a value of type MyState.
I added a comment that describes our case: reduxjs/redux#3580 (comment)
There was a problem hiding this comment.
Thanks for pointing out that Redux issue thread @jsnajdr, we're not the only one's grappling with how Redux and TS should work together.
I took a look at the solution Simplenote is using, and their typings suffer from the same issue i.e. it's possible to write a reducer with no type errors that doesn't deal with unknown action types.
This isn't ideal, but given it's something community hasn't figured out yet (the approach recommended by the docs has the issue) is this something where we just wait and see where Redux goes with this. This PR improves things a little at least by widening the action type.
p-jackson
force-pushed
the
update/data-store-action-types
branch
from
a86aad8 to
459a4ac
Compare
sirreal
force-pushed
the
update/data-store-action-types
branch
from
459a4ac to
cd2dcbe
Compare
This comment has been minimized.
This comment has been minimized.
|
Rebased. I accidentally posted the previous incomplete comment. I've continued to have higher priority things get in the way of coming back to this. I was hoping to leave detailed feedback and propose some changes and improvements, but I've been unable to find the time. In general I think there are opportunities for simplification around here. I'm not sure I like the ergonomics of the new mapped types. Needing to import the namespace There also seems to be opportunity for improvement with the type of the type property of actions. I'd like to remove the action-type enums, which don't make sense with a small set of action creators. I started exploring and working on some ideas which I've shared in #39259, but I haven't found the time to finish it up. |
|
Thanks for commenting @sirreal, appreciate that you're trying to balance priorities :) fwiw I like removing the enum for action types, I don't really feel adding the namespacing adds any value. Interesting idea about removing the mapped type and explicitly defining a union of all action types. One advantage I see is that if some hypothetical action had a complicated enough type then we might want to define it explicitly as its own interface, instead of infer it from the action creator's return type, then it could easily be included in the union type. |
p-jackson
force-pushed
the
update/data-store-action-types
branch
2 times, most recently
from
584fab7 to
88c2331
Compare
The extra namespacing the `ActionType` enums provided aren't worth the extra typing. It makes the already long identifiers even longer, and you already get auto-complete in reducers because TypeScript knows what values are possible when you type `action.type ===` in a reducer.
p-jackson
force-pushed
the
update/data-store-action-types
branch
from
88c2331 to
dca75ac
Compare
|
Rebased to include the new site store. I've updated to take some ideas from @sirreal's branch:
|
There was a problem hiding this comment.
Overall I'm quite happy with the direction, thanks for the initiative!
As noted, I do see reason to continue using Reducer or add explicit return types to the reducer function declarations. What do you think?
I've tested and there don't seem to be any regressions. Approving with the note that I'd like to see the above change 🚀
| type DomainSuggestion = import('@automattic/data-stores').DomainSuggestions.DomainSuggestion; | ||
| type Template = import('@automattic/data-stores').VerticalsTemplates.Template; | ||
|
|
||
| function domain( state: DomainSuggestion | undefined, action: OnboardAction ) { |
There was a problem hiding this comment.
I like using the Reducer type because it ensures we return a state type. I'd say it's a bit more cumbersome here using function declarations:
| function domain( state: DomainSuggestion | undefined, action: OnboardAction ) { | |
| function domain( state: DomainSuggestion | undefined, action: OnboardAction ): DomainSuggestion { |
If we stay with const declarations and Reducer, we get the following which seems like a good place:
| function domain( state: DomainSuggestion | undefined, action: OnboardAction ) { | |
| const domain: Reducer<DomainSuggestion | undefined, OnboardAction> = ( state, action ) => { |
If we don't return the same type of state, that's a bug types can help detect.
There was a problem hiding this comment.
I've dropped the commit that converted the reducers to function expressons. Given all selectors and action creators are using the const way it doesn't make sense for the reducers to be any different.
I'd say it's a bit more cumbersome here using function declarations
Given that I think it's less cumbersome this way goes to show it must be a preference thing, so better to just go with the flow of what's already there.
Then again I did forget to specify the return types, so maybe you have a point 😅
There was a problem hiding this comment.
I don't have much of a preference as long as we're getting well typed reducers (with return):
const r: Reducer< State, Action > = ( s = initialValue, a ) => s;
// or
function r_( s: State = initialValue, a: Action ): State { return s; }| export function* createAccount( params: CreateAccountParams ) { | ||
| yield fetchNewUser(); | ||
| try { | ||
| const newUser = yield { | ||
| type: ActionType.CREATE_ACCOUNT as const, | ||
| type: 'CREATE_ACCOUNT' as const, | ||
| params, | ||
| }; | ||
| return receiveNewUser( newUser ); | ||
| } catch ( err ) { | ||
| return receiveNewUserFailed( err ); | ||
| } | ||
| } |
There was a problem hiding this comment.
When working on this, this had me curious:
- There's an untyped literal action object that's yielded in the middle. An observation, not necessarily an issue.
- This is a generator, which uniquely types its yield and return types.
I'm not sure whether our reducers "see" yielded actions (presumably handled by controls?) or only returned actions. Maybe this is up to the handling control and not something we can generalize about.
There was a problem hiding this comment.
Just tried this and was surprised to see that the reducer didn't see the 'CREATE_ACCOUNT' action 🤔
There was a problem hiding this comment.
@p-jackson I ran into this when I was working on the Site data store, and it looks like the way the redux-routine middleware works is that if it matches a control against the name of an action, it runs the control but doesn't dispatch the action to pass it along to the reducer. For unhandled actions (actions that don't have a match in the controls), they do get dispatched by the middleware so that they hit the reducer as normal.
I still don't think I have a solid grasp of exactly how the middleware works, but in effect, it feels like controls are designed to intercept an action, and then in the control we can then dispatch actions that will hit the reducer. Because the reducer doesn't see the action that's listed in our controls, that's why we've got the additional fetchNewUser action that gets yielded before CREATE_ACCOUNT, otherwise from the reducer, we don't know that the action has started. It seems like useful behaviour for a middleware to me, but definitely non-obvious in its behaviour!
p-jackson
force-pushed
the
update/data-store-action-types
branch
from
dca75ac to
51f2df3
Compare
|
|
||
| type Template = VerticalsTemplates.Template; | ||
|
|
||
| export const setDomain = ( | ||
| domain: import('@automattic/data-stores').DomainSuggestions.DomainSuggestion | undefined | ||
| ) => ( { | ||
| type: ActionType.SET_DOMAIN as const, | ||
| type: 'SET_DOMAIN' as const, |
There was a problem hiding this comment.
This is just a question for my own curiosity (and wrapping my head around TS features), what's the value of adding as const on a string literal? Does this prevent the type key from being changed?
There was a problem hiding this comment.
Without it the type prop would still be a string, not a string literal unfortunately.
Another interesting way of doing it is:
return <const>{
type: 'SET_DOMAIN',
domain,
};Which not only makes typeof type the correct string literal, but it also marks all the props as readonly.
Hover your mouse over the types in this playground example to see it in action.
There was a problem hiding this comment.
Ah, gotcha! Thanks for the example, that makes it clear to me now :)
There was a problem hiding this comment.
I'd like the reducers to all return { … } as const if anyone wants to take that task on 👍
There was a problem hiding this comment.
This PR's looking great to me @p-jackson — thanks so much for sticking with it and rebasing against the Site data store, too, it makes the reducers much more readable to me while also better reflecting how the reducers function! 👍
matticbot
removed
the
[Status] Needs Review
In order to get type checking and code completion, the data-store reducers have been explicitly listing each of the actions they handle in their type signature. I think this (1) doesn't scale well as more cases are added to the switch statement (2) makes the reducer's type too specific.
A reducer should be able to handle any of a store's actions without outside observers being aware any changes that are made. By listing them in the type they're effectively part of the function's API, and changing the implementation could be a breaking API change (loosely speaking).
Edit: and converting the reducers to function expressions is entirely optional. I just did it to drive home that adding TypeScript to reducers doesn't require you to jump through any extra hoops, write reducers in a way that's different from JS reducers, or declare somethings type before defining it.
But I don't mean to bikeshed on a stylistic thing 😅 I made the change in a single commit so it's easy to drop.
Changes proposed in this Pull Request
Reducer action types are still generated automatically from the action creators, but now it includes all actions.
Add a newActionsDefinedInModulemapped typeSwitch the reducers to use function expressions (optional)