Avoid using a memoized selector without dependencies #57257
Conversation
youknowriad
added
the
[Type] Bug
youknowriad
requested review from
draganescu,
talldan,
noisysocks,
tellthemachines,
adamziel and
kevin940726
as code owners
|
Size Change: -441 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
As per #57198 (comment), I don't think this is the right fix. I think it's important to also note that this broke the redux devtools compatibility in the PR description. It seems like it also crashes the whole page when the devtools is active. The console only outputs |
|
I'm happy if we find an alternative fix that has the following properties:
I'm open to suggestions. |
Have you read my latest comment in the original issue? I don't think this is solvable without introducing some form of a public API. Consider the site editor's tree: <Header />
<EditorProvider>
<ListViewSidebar />
</EditorProvider>If we want to allow implicit state communication between <Header />
<Header />
<EditorProvider>
<ListViewSidebar />
</EditorProvider>
<EditorProvider>
<ListViewSidebar />
</EditorProvider>How would any of the instances know which
We have to provide some "context" for them to be able to share the one-to-one relationship. This will be some form of a "public API". Unless we only allow one pair of them to be rendered, then all of it doesn't matter. But then we can just create a module-level Or, we only allow Should we consider reverting this PR and #57198 until we find a better solution? Given that not only did they not solve the issue, but also broke the devtools compatibility (worse DX). |
EditorProvider renders a nested store unless So for the default third-party developer, it's just not supported to render Header outside of |
Do you mean it's never supported to render Maybe I'm missing something, but why couldn't we just use regular React context and fallback to a module-level ref? const defaultListViewToggleRef = createRef();
const ListViewToggleRefContext = createContext();
function DocumentTools() {
const listViewToggleRef = useContext(ListViewToggleRefContext) ?? defaultListViewToggleRef;
return <ToolbarItem ref={listViewToggleRef />;
}
function ListViewSidebar() {
const listViewToggleRef = useContext(ListViewToggleRefContext);
function closeListView() {
const listViewToggle = listViewToggleRef.current ?? defaultListViewToggleRef.current;
listViewToggle?.focus();
}
}To me though, the inconsistency between third-party usage and WP usage is the most focusing part. It seems to me the easiest and most idiomatic way is to expose a private API in function EditorProvider({ _privateListViewToggleRef }) {
const listViewToggleRef = useRef();
return (
<ListViewToggleRefContext.Provider value={_privateListViewToggleRef ?? listViewToggleRef}>
{children}
</ListViewToggleRefContext.Provider>
);
} |
This IMO is just a weird design decision that I think we're going to revert soon anyway. Basically we want to animate the header from the top of the window in the site editor and while it's possible to do it while rendering the Header within the "canvas/frame", it's more convenient to do it when the header is just at that position from the start. So right now the Header is rendered outside for this reason but for me conceptually, that header belongs to the frame/canvas and should be within
Yes, that's possible but we already have one it's the Editor store, it's just a React context under the hook (aka RegistryProvider), and adding yet another context to main while we have already one felt useless to me. Dogma (aka twisting a bit what the reducer is meant to contain) is rarely a strong argument for me (we need to understand the spirit of the rule if I dare to say). |
I don't think it's useless if it actually serves some purpose here 😆. It's also invisible to users/developers and provides next to no performance penalties.
I don't think "dogma" is a fair word to use here. I discovered and debugged the issue, stated the concerns, and provided multiple alternative solutions. I'm never against bending the rules sometimes, but IMHO we should have good reasons and good documentation for people to follow (so that people won't blindly copy the practice everywhere else where it doesn't apply). For instance, we don't know whether Redux or Redux Devtools would someday introduce a feature that doesn't work with non-serializable data. Note that Redux Devtools updates itself automatically so it would be a surprise for us if it happens. (I doubt that it would happen though.) That said, I don't feel strongly about the solution we use, as long as the majority of contributors are comfortable with it 👍. I just feel that it's important to share my opinions and concerns here for visibility. 🙂 |
I think breaking the standard debugger of one of the biggest pieces of the stack we use should trigger some consideration of the above, aside from all the other issues discussed here in terms of API design. |
Addresses an issue raised here #57198 (comment)
I initially thought that a memoized selector would give a different value per "store" if there's no dependencies. It turns out that assumption is wrong and that memoized selectors without dependencies means it's just a global function.
To address this I'm restoring the "static" reducer, I had implemented initially.
Testing instructions