ListView: Replace prop drilldown by a stable ref in store #57198
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
the
[Type] Code Quality
youknowriad
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Size Change: +579 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Dec 19, 2023
packages/editor/src/store/reducer.js
Outdated
| * @return {Object} Reference to the list view toggle button. | ||
| */ | ||
| export function listViewToggleRef( state = { current: null } ) { | ||
| return state; |
There was a problem hiding this comment.
As we add non-serializable data into the store we break the compatibility of Redux Devtools. It's also a mutable reference which to me might have more suitable place for it. We might have to decide whether we want to completely drop support of Redux Devtools as a debugging tool or we could just move this to something simpler like a ref object?
export const listViewToggleRef = React.createRef();There was a problem hiding this comment.
Where would you put that ref object? We want something unique per editor instance (so per editor store).
There was a problem hiding this comment.
Actually, there's no need for reducer, we can just use a memoized selector.
There was a problem hiding this comment.
Couldn't we just expose it as a private api or just define it at the module level for each editor? Why making it a selector if it's not selecting anything from the store?
There was a problem hiding this comment.
We can't define at the module level because if you have multiple EditorProviders (multiple editor instances), it breaks. What we could do is use another "context provider" but for me this is counter productive, the store is a context provider on its own.
I agree that it's weird but it does the job without any downside.
There was a problem hiding this comment.
Interesting, so you're saying that two memoized selectors from different store instances will return the same values if they both have the same "dependencies"? I feel like you might have uncovered a bigger bug here :). Shouldn't memoized selectors be specific to the store they're used with?
There was a problem hiding this comment.
You're passing an empty dependencies array so that basically means never revalidates the cached value. If you pass in [state] though then it will revalidate every time any change is made to the store. I don't know the exact problem we're solving here so can't provide further feedback, but I still think we should keep it simple and idiomatic here. I don't think prop drilling is that bad.
There was a problem hiding this comment.
I disagree to be honest, Prop drilling is bad because it means introducing a bad public API here. I'm going to restore the reducer as a follow-up to prevent the issue you raised. It's not important for this particular reducer to be tracked in redux dev tools. If we figure out an alternative that doesn't involve adding new contexts or new APIs, I'm all for it.
There was a problem hiding this comment.
I've opened a follow-up here #57257
And to clarify further. Both the "header" of the editor and the "list view" are going to be refactored as components of the "editor" package to unify both post and site editors. This means if we want to keep prop drilling, we'll have to have a public API in EditorProvider or something just to share this prop between the two components (because they're both rendered by the Layout -> Header ... And for me that's a no go. This is an internal implementation detail that shouldn't leak to the public API of the editor package.
There was a problem hiding this comment.
Thanks for the extra context! But I still don't understand the use case here 😅.
Unless we register two different editor stores, we'd still be accessing the same ref object even when we render two different EditorProvider. We can achieve this by using the useSubRegistry prop, but then the sub registry only exists in that part of the React tree. If the Header component can be rendered outside that part of the tree then how would the Header know which editor and which listViewToggleRef it should connect to? If we want to link the connection between Header and EditorProvider then there has to be some public API or a higher-level context provider.
It's not important for this particular reducer to be tracked in redux dev tools.
I'm afraid the default behavior is not as simple as ignoring that part in the debugging tool, sometimes it just crashes the devtools or even the whole page, even when the Redux devtools tab is not active.
ntsekouras
approved these changes
Dec 19, 2023
bph
added
the
[Feature] List View
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #52632
What?
This is a small refactor to avoid prop drill down for the "listViewToggleRef". We were using a local ref for this which has two downsides:
This is also a preparation PR to move to align list view between post and site editors.
How?
The trick is to use the store to keep track of a single mutable "ref" and just have a private selector to retrieve this ref.
Testing Instructions
1- You can open and close the list view and the focus will be restored properly to the toggle button in post, site and widget editors.