NavigatorProvider: move all state management to one reducer #60190
Conversation
jsnajdr
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
jsnajdr
force-pushed
the
update/navigator-provider-state
branch
from
98769f7 to
8537f21
Compare
There was a problem hiding this comment.
Left some questions, but nothing blocking - this looks amazing 👏
The new reducer version is more concise and readable and can help us make improvements and additions easier.
I've spent some extensive time testing both in the site editor and in storybook, and couldn't manage to break it.
Great work @jsnajdr 🚀
| } | ||
|
|
||
| return state; | ||
| // Return early in case there is no change |
There was a problem hiding this comment.
Can we return even earlier in a default case when the action type was not one of the ones we care about? Or are we relying on the types to do their job to prevent that from happening?
There was a problem hiding this comment.
This is not a public reducer, it's not part of a larger tree and doesn't need to accept an action of any type. We have all the action dispatches under tight control.
If you dispatch some bogus action, the reducer will still behave correctly, the "early return" check will work.
| : undefined; | ||
|
|
||
| // If the new match is the same as the previous match, | ||
| // return the previous one to keep immutability. |
There was a problem hiding this comment.
Thanks for adding and improving comments, they're really helpful 🙌
| } from '../types'; | ||
|
|
||
| type MatchedPath = ReturnType< typeof patternMatch >; | ||
| type ScreenAction = { type: string; screen: Screen }; | ||
| type RouterAction = |
There was a problem hiding this comment.
I assume you don't expect these types to have any utility outside of this module, which is why you opted to keep them private?
There was a problem hiding this comment.
Yes, the reducer is private to the component, it's all self-contained in this file.
| state: Screen[] = [], | ||
| action: ScreenAction | ||
| ): Screen[] { | ||
| type RouterState = { |
There was a problem hiding this comment.
Nit: I'd move this type up, next to the other types.
| | { type: 'add' | 'remove'; screen: Screen } | ||
| | { type: 'goback' } | ||
| | { type: 'goto'; path: string; options?: NavigateOptions } | ||
| | { type: 'gotoparent'; options?: NavigateToParentOptions }; | ||
|
|
||
| const MAX_HISTORY_LENGTH = 50; |
There was a problem hiding this comment.
I couldn't help but wonder what's the rationale behind keeping such a short history, and whether it would be costly to increase it. Could potentially turn useful as the router gets used more and more. Not something we should aim for in this PR of course.
There was a problem hiding this comment.
This is probably not needed. There are createMemoryHistory functions both in history and in @tanstack/react-router, and neither does bother to limit the history size. You can freely push new items to the array with no limit.
I'll do a followup that removes this. I also suspect that the replace action doesn't work very well.
| dispatch( { type: 'add', screen } ), | ||
| removeScreen: ( screen: Screen ) => | ||
| dispatch( { type: 'remove', screen } ), | ||
| } ), | ||
| [] |
There was a problem hiding this comment.
Should dispatch be a dependency of this useMemo()? In practice, it won't make a difference since dispatch is a stable reference and that useMemo() will be called just once, but still, could be useful to pass it to satisfy the linter.
There was a problem hiding this comment.
The linter shouldn't complain here; it's aware that native setState and dispatch have stable references.
| // Compute the matchedPath | ||
| const currentPath = | ||
| locationHistory.length > 0 | ||
| ? locationHistory[ locationHistory.length - 1 ].path | ||
| : undefined; | ||
| matchedPath = | ||
| currentPath !== undefined | ||
| ? patternMatch( currentPath, screens ) | ||
| : undefined; | ||
|
|
||
| // If the new match is the same as the previous match, | ||
| // return the previous one to keep immutability. | ||
| if ( | ||
| matchedPath && | ||
| state.matchedPath && | ||
| matchedPath.id === state.matchedPath.id && | ||
| isShallowEqual( matchedPath.params, state.matchedPath.params ) | ||
| ) { | ||
| matchedPath = state.matchedPath; | ||
| } |
There was a problem hiding this comment.
Looks so much cleaner and readable like this 👏
jsnajdr
force-pushed
the
update/navigator-provider-state
branch
from
c3f1bd5 to
c187d7b
Compare
…s#60190) * NavigatorProvider: move all state management to one reducer * Changelog update * Move types together Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
…s#60190) * NavigatorProvider: move all state management to one reducer * Changelog update * Move types together Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org> Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
This PR refactors the routing logic in
NavigatorProvider. It removes all theuseRefs anduseEffects, and moves all state management to auseReducerhook. There is now one reducer to rule them all: the list of routes inscreens, the navigation history inlocationHistory, and the currently matched route inmatchedPath. They are all updated on action dispatches. The only job left to do forNavigationProvideris to create a very simple binding of the reducer state to a context value.Makes the
NavigatorProvidereasier to understand and also to replace it in the future with some standardized router.How to test: Everything should work the same way as before: try out various routing scenarios in Site Editor and other places.