Replace turbo-combine-reducers with combineReducers from Redux - #54606
Conversation
jsnajdr
added
[Type] Performance
| } | ||
| setIsListViewOpened( showListViewByDefault ); | ||
| setEditorCanvasContainerView( undefined ); | ||
| setEditorCanvasContainerView( null ); |
There was a problem hiding this comment.
Note that changing all reducers to use undefined instead of a null is likely to end up being a regression, since some of the reducers might make a strict differentiation between the null and undefined. This is especially valid for custom @wordpress/data stores that are already using undefined as a valid value somewhere in their reducer trees.
With that in mind, I think we might have to consider adding our own wrapper around the Redux combineReducers that handles undefined in a way that allows supporting it as a valid value.
There was a problem hiding this comment.
@jsnajdr what are your thoughts on the above comment?
There was a problem hiding this comment.
Now, after I abandoned the idea to use combineReducers from Redux and created my own version instead, undefined state is no longer a concern. And neither are extra keys in state. @wordpress/data reducers can continue to use them without any change.
However, this PR still contains changes to Gutenberg's data stores that remove undefined state and replace it with null. They're no longer strictly needed, but still seem to me like a good code quality improvement.
But these changes are not worth doing if they introduce any backward compatibility issues for the existing Gutenberg stores. But are there any? And if there are, can they be solved?
For example, the setEditorCanvasContainerView( undefined ) call no longer works and has to be changed to setEditorCanvasContainerView( null ). But that can be easily avoided by defaulting the action parameter to null: ( view = null ) => .... I haven't 100% verified this yet, I only know that unit and e2e tests are green.
|
Size Change: +24 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
|
Here are performance tests results. I don't see any significant regressions. Some tasks are faster, some are slower, and my intuitive guess is that none of them is outside the random fluctuation threshold. |
d3d2537 to
d1d71e8
Compare
|
Flaky tests detected in d1d71e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6298235508
|
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
This is great work, thanks for working on it @jsnajdr!
As we discussed separately, it might be a good idea to move the undefined -> null changes to a separate PR, since they might be controversial and potentially breaking for some use cases.
I also think we can add some unit tests to cover our custom combineReducers implementation.
Once those two points are addressed, I think we should be good to go.
d1d71e8 to
ce4de3a
Compare
There was a problem hiding this comment.
This is a fix that was forced by the "null state instead of undefined" changes, and it can be merged even though the state.currentTheme remains undefined. getEntityRecord should never get a fourth argument (key) that's not a valid string, its type doesn't allow that. The State type for the getCurrentTheme's state argument "lies" about the type of state.currentTheme. It can be undefined (or null) when there is no active theme or when the active theme is not yet fetched from the REST endpoint.
There was a problem hiding this comment.
I realize this is pre-existing, but there are places in the codebase that rely on the getCurrentTheme() to always return a non-nullish value, for example:
Should we be concerned that those potentially might cause issues under some conditions?
There was a problem hiding this comment.
If state.currentTheme is not a valid theme slug, then getEntityRecord would return undefined anyway, because the state never has a theme whose slug (ID) would be undefined or null. So, it's not that we're now returning null where a theme object would be previously returned.
The existing places in the codebase are incorrect, because getCurrentTheme() is not guaranteed to return a valid theme. The code relies on the fact that by the time it runs, the /wp/v2/themes?status=active REST request has already finished loading.
There was a problem hiding this comment.
Agreed - the existing code would throw an error before with the undefined, and will throw an error now with the null.
IMHO these cases can be handled in another PR, and are something that an eventual TS migration will catch anyway.
There was a problem hiding this comment.
Related to the state.currentTheme fix. The stylesheet field contains the ID of the theme, and if the test mock doesn't supply it, the state.currentTheme becomes undefined.
Both points are addressed now: I removed the changes for |
There was a problem hiding this comment.
I realize this is pre-existing, but there are places in the codebase that rely on the getCurrentTheme() to always return a non-nullish value, for example:
Should we be concerned that those potentially might cause issues under some conditions?
There was a problem hiding this comment.
I think this needs a CHANGELOG entry.
There was a problem hiding this comment.
Good point, added a changelog entry for the data package.
There was a problem hiding this comment.
This is more or less a simplified version of the Redux combineReducers(): https://github.com/reduxjs/redux/blob/957aed6e23624113b4d0ff6d9f277f25fca90b38/src/combineReducers.ts#L123
Should we add a mention in CREDITS.md?
There was a problem hiding this comment.
Probably not, I think it's sufficiently different from Redux. There are only so many ways to write a combineReducers function, it's very hard to be completely original 🙂 Also, Gutenberg doesn't even have a CREDITS.md file.
There was a problem hiding this comment.
Redux's combineReducers will also complain if there are no reducer subkeys. We could add a test for that too, although it would be a rare scenario.
There was a problem hiding this comment.
I don't understand, do you mean calling combineReducers( {} )? I think that's a valid case, it returns a valid "zero" reducer that always returns {}. Something like empty {} TypeScript type.
There was a problem hiding this comment.
Yes, I was talking about combineReducers( {} ). AFAIK the Redux version won't allow this, while our version allows it. So that makes it a good case for a unit test IMHO.
There was a problem hiding this comment.
Should we add a failproof check to ensure reducers is a proper object? Should be cheap IMHO. If you agree we should, it could be yet another case to add a test for.
Alternatively, we could introduce combineReducers as a typed function. Then we wouldn't be worried about non-object reducers arguments.
There was a problem hiding this comment.
Neither Redux nor turbo-combined-reducers do such a check on reducers. Both of them do an Object.keys( reducers ) call at the very beginning, that's where they crash if it's not a proper object.
@wordpress/data re-exports the Redux type for combineReducers, so the function has been already typed, and continues to be.
There was a problem hiding this comment.
Agreed, no need to overcomplicate 👍
ce4de3a to
3ee400f
Compare
It's added, now I'm going to 🚢 |
|
After installing this update, I'm seeing a lot of errors like |
|
Fix for the type problem is here: #55321 |
3 participants
This is still an experiment, I'm opening a PR mainly to get performance test results.
There are two major problems with
combineReducersin Redux.First, it doesn't allow slice reducers to ever return
undefinedstate.undefinedmeans uninitialized and every reducer is supposed to return an "initialized" inital state whenundefinedstate is passed. We have many reducers that useundefined, and this PR has to modify them to usenullinstead. Otherwise,combineReducerswill throw an exception and the app will crash. The prohibition onundefinedwill be a breaking change that we probably can't afford to do.Second, it logs a warning when the combined reducer function is called with a state that contains unknown keys, with no reducer for them. For example, consider combining
fooandbarreducers withcombineReducers( { foo, bar } ). Then, when you pass an object with shape{ foo, bar, baz }asstate,combineReducersdoesn't like thebazfield, because there is no sub-reducer to pass it to. The warning is:The
core/blocksstore very often uses an enhancer pattern that leads to these "unexpected key" warnings:This enhancer calls the inner reducer and then adds the
isIgnoredChangefield to the existing ones. If the inner reducer is a result ofcombineReducers, it will complain about the unknown key. Although everything works, there's nothing really wrong with this pattern.For these two reasons, we'll probably have to write our own version of
combineReducers.Fixes #42513.