Migrate the 'editor' package to builtin data controls #25990
Conversation
jsnajdr
added
[Package] Editor
|
Size Change: +12 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
It feels like it shouldn't be the consumer's responsibility to think about this. Why not always use resolveSelect and make sure that it resolves directly if the selector has no resolver. |
That's possible -- But I feel the programmer should be aware of the difference between these two types of selectors, as they are conceptually two different things. Selector without a resolver is basically a glorified variable in memory that can be read and assigned to, in a React-compatible way. Selector with a resolver means there is a remote request that needs to be resolved asynchronously. It can fail and the result it stored locally in a cache (that all what the local reducer and store really is -- cache for these requests) In the future, I think both types of selectors will be accessed through two distinct APIs, and each of them will feel natural for the purpose. At this moment, the difference between |
I think that's a big shift for the data package. It was modeled from the start that components declare their data needs (select) and don't care whether it's sync or async, it's an implementation detail the component user should rarely think about. (granted it's not always true). Based from this principle, it seems coherent to always use resolve select in controls IMO. Now, whether that assumption/principle is to be rediscussed/rethought is a big decision. |
jsnajdr
force-pushed
the
migrate/editor-builtin-controls
branch
from
d827f3d to
c838bdd
Compare
|
Rebased and resolved conflict with #20149 (stabilizing the local autosave API) |
Maybe I misunderstand something, but a React component author must be aware about the async nature of a selector with resolver. It's not possible to write code that works otherwise. As a random example, consider the The component needs to handle the case where the selector returns Compare that to how the
It's true that when we use a selector-with-resolver in a control, we almost always want the resolved value. The control typically implements some async flow (do this and then do that), and the flow is ran only once -- there's no opportunity to "react" to store changes and rerun it with updated values. But I'm afraid that always using Consider the fix that @epiqueras did in #21839 to fix race conditions in the That's a typical example that the programmer had better be aware about sync/async even when writing controls.
First of all, I believe this assumption/principle simply doesn't hold in the data package as it is today. The user of the API needs to be aware of the sync/async nature of things if they want to write reliable code. Second, I don't think that the ambitions I described are that radical 🙂 An example of one thing I'd like to see changed: the const { siteIconUrl, isRequestingSiteIcon } = useSelect( ( select ) => {
const { getEntityRecord, isResolving } = select( 'core' );
return {
siteIconUrl: getEntityRecord( 'root', '__unstableBase', undefined )?.site_icon_url,
isRequestingSiteIcon: isResolving( 'getEntityRecord', [ 'root', '__unstableBase', undefined ] ),
};
}, [] );This code is a bit unwieldy and repetitive, and I'd like to arrive at some much more elegant API to express the same thing. That's shouldn't need any radical rewrite -- just some incremental refactorings and then maybe one new hook that builds on top of |
Migrates the
editorpackage to the builtin data controls introduced in #25362.Builds on top of #25987 that improves some of the affected tests.
While reviewing and testing, particular attention should go to the choices between the sync
selectand asyncresolveSelectcontrol. The only selector where we need to wait for resolution isselect( 'core' ).getPostType(). Everything else is sync. All selects fromcore/editorin particular are always sync, because the store doesn't have any resolvers.