core-data: use controls from data-controls in favor of the package-local ones #25235
Conversation
…cal ones The generic `apiFetch`, `select` and `dispatch` controls are frequently duplicated in various stores. This patch fixes that for the `core-data` package.
jsnajdr
added
[Type] Code Quality
|
Size Change: -191 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
|
I agree naming is unfortunate. Such is what happens as changes evolve over time. I don't think we can change the naming right now for back-compat reasons but happy to be wrong if Riad has ideas. I'm okay with the other suggestions regarding graduating unstable/experimental APIs but I'd defer to Riad for the final call here.
That does sound like it could be good for the use-case where consumers want the current state without triggering any potential resolvers. I think |
|
Same here, I don't think we can rename now the stable ones. I'm ok with making the experimental APIs stable here. Maybe we want to wait before introducing peekSelect (wait for a concrete use-case in Gutenberg) |
One approach that fixes a big part of the damage while keeping back compat:
OK, I can do it in another PR. Keeping this one focused on introducing
I definitely agree with not doing it in this PR, if that's what you mean by "waiting". Regarding the concrete use-case, I believe it's already there in |
|
I'm not sure I like the renaming back-compat approach because I like the parallel between |
From a Redux purist perspective, triggering the resolver as a side-effect of Triggering the resolver is also problematic because it's done during the render phase. It's incompatible with React async rendering: React can discard the render results and never commit them, and yet the side effect will be triggered. It also causes the new "cannot update component while rendering component" React warning and workarounds were needed in #21289. I believe this is an issue with the current Aside from Redux, the concept of "selector with a resolver" is OK, and libraries like Recoil use it: My main concern with the naming is that the names of the controls don't correspond to the store methods they are wrappers for: the |
The API is meant to be "React-agnostic". If we do this change, it means we're changing the semantics of |
I think it's posible to keep For regular For React bindings, we want to batch the resolver calls and fire them all later in a This is a possible implementation of const useSelect = ( mapSelect ) => {
const registry = useRegistry();
const { scheduler, select } = registry.selectWithScheduler( ReactScheduler );
const mapOutput = mapSelect( select );
useEffect( () => {
scheduler.flush();
} );
return mapOutput;
}The I'm not sure if this can ever really work, especially in a large React tree where components interact, but it looks like it can be done. |
The generic
apiFetch,selectanddispatchcontrols are frequently duplicated in various stores. This patch fixes that for thecore-datapackage.How to test:
core-datastore is used at a lot of places, it's hard to point at any single feature.Note:
I think the naming of the select controls in the
data-controlspackage is very unfortunate:selectdoesn't do the synchronousselect(return whatever is in the store right now) that we all know from Redux orwithSelectoruseSelect, but does the promise-returning resolution based on__experimentalResolveSelect. This control should really be calledresolveSelect.__unstableSyncSelecthas an unwieldy name and should be calledselector at leastsyncSelect. I don't see anything that should be unstable or experimental about this control. It's the most non-controversial and straightforwardselectI can think of 🙂@youknowriad @nerrad Is it possible to fix that naming, or are we stuck with it forever, for back-compat reasons?
It also seems to me that
__experimentalResolveSelectshould be graduated from the experimental state. It's being used in many async workflows that do several async requests sequentially and they depend on each other. Example: fetch info forpostTypeand then use itsrestBaseto fetch autosaves for it.Another thing that would deserve first class support is calling a selector while skipping side-effects: something that
__experimentalGetEntityRecordNoResolverdoes. That could be calledpeekSelect.Our selectors and their resolvers are conceptually very similar to selectors in Recoil. There, every "node" has API like:
get(): return the value if resolved or throw a promise if unresolved (this is optimized for Suspense, but conceptually the same thing as ourresolveSelect)getLoadable(): return a{ value, state }object wherestatecan be one ofloading,hasValue,hasError. Similar to synchronousselect. Triggers resolution.peekLoadable(): same asgetLoadablebut doesn't trigger resolutioninvalidate(): drop the cached resolved value -- nextget()will trigger new resolution.