DataViews: Support passing the registry to actions callbacks #62505
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
oandregal,
jsnajdr,
jorgefilipecosta and
ntsekouras
|
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. |
packages/dataviews/src/types.ts
Outdated
| callback: ( items: Item[] ) => void; | ||
| callback: ( | ||
| items: Item[] | ||
| ) => void | ( ( { registry }: { registry: any } ) => void ); |
There was a problem hiding this comment.
Unfortunately, we can't be more specific for the registry type just yet because the data package is not fully typed yet.
|
Size Change: +33 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Are there backward compatibility concerns for this API? Like, do we need to keep supporting the existing non-thunk actions with existing parameters? What this PR really does is that it adds support for passing a const callback = ( context, items ) => { /* ... */ };or curried: const callback = ( items ) => ( context ) => { /* ... */ };The proposed approach feels like a patch that was added as an afterthought. To explain the API you need to say things like "in the callback function, perform your work, but you can also return another function with more work and we will call that one, too, with additional context the original function didn't have." That's a lot, you should be able to say just "callback with two parameters". Also the thunk is a bit error prone if you implement a callback like: callback: ( items ) => callSomeAPI( items[ 0 ] )then if |
|
There are no backward compatibility concerns here at all but your idea of passing "context" (registry) as an extra argument is interesting. I actually just didn't think about it because I just mapped the API here with the Redux Actions we have. 🤔 |
|
I think thunks are a good idea when the plain action is just an inert data object. Then a thunk is a fundamentally new thing that you can dispatch in addition to plain actions. But here both non-thunks and thunks are functions, just with different parameters. Better to unify them into one function type, with the same parameters. That's how I think about it 🙂 |
|
Thanks for elaborating. You're right, it does seem simpler. I'm going to make the change. |
| context: { | ||
| registry: any; | ||
| onActionPerformed?: ( items: Item[] ) => void; | ||
| } |
There was a problem hiding this comment.
Is the onActionPerformed field used anywhere? I see that all calls pass just { registry }.
Is it possible to make context the first argument? It's something like state for a selector or a this argument when defining type for a TS function. It will look odd when you later add a third or four parameter and context will be randomly somewhere in the middle.
There was a problem hiding this comment.
I think it makes more sense to put the context as the last argument since it's not really necessary for the callback. The "items" is more important.
Also, one of the reasons, I made it an object is to be able to add more optional configs later there.
For the onActionPerformed, yes it's used in both post and site editors (but not in dataviews). I don't love this API personally, but I don't want to change everything here. It can potentially be made unnecessary in callback actions by making these functions return promises but it will be necessary in Modal Actions as a prop, so maybe it's fine to keep it for callback actions as an argument too.
There was a problem hiding this comment.
The downside is that if you ever add new parameters (how likely is that?) then context will have to move or will end up in a weird position. 🤷♂️
youknowriad
force-pushed
the
support/action-thunks-in-dataviews
branch
from
c6e2ece
to
b8996d6
Compare
| callback: ( | ||
| items: Item[], | ||
| context: { | ||
| registry: any; |
There was a problem hiding this comment.
From the point of view of a consumer that wants to use the @wordpress/dataviews package in its own screens, what's a registry?
I lack context on the way we are evolving actions and how this helps, so I'd like to learn more why we want to tie wordpress/dataviews to wordpress/data concepts.
There was a problem hiding this comment.
There was a problem hiding this comment.
Registry just allows to any wordpress store. It doesn't introduce a dependency to a particular store but it allows dataviews actions to trigger actions from any store.
We used to that by having "React hooks" that actually return action but that is not ideal IMO. I prefer for dataviews actions to be global registrable objects just like blocks for instance.
There was a problem hiding this comment.
Oh yes I understand the complaint and I think it's very valid 🙂 This PR makes the dataviews library dependent on data, even though it doesn't really use it. It just retrieves the registry and passes it to actions. And most actions won't use the registry at all. But the data library still needs to be present because the useRegistry calls are happening all the time.
A much more flexible approach would be if the context was empty by default, and you would configure it when creating a dataview. I'm not very familiar with dataviews, but something like this might work:
const registry = useRegistry();
const actionsContext = useMemo( () => ( { registry } ), [ registry ] );
return <DataViews
actions={ actions }
actionsContext={ actionsContext }
/>A certain app like edit-site wants to pass registry to its actions, so it configures its DataViews accordingly. The library is completely unaware about what's in the context, and other apps can put something completely different there.
There was a problem hiding this comment.
Jarda, your example illustrates a lot better the point, thanks. I still like to clarify why the "context" needs to be passed through the DataViews API if it is not used/modified by it. Cannot the actions retrieve whatever extra data they need?
const actions = [
{
id: 'view',
callback: ( items, { onActionPerformed } ) => {
// onActionPerformed is triggered by DataViews.
// The action is responsible to retrieve whatever else it needs.
}
}
];
return <DataViews
actions={ actions }
/>I understand a valid use case for having to pass something through would be the following: the action's callback needs to have something ready when it is called. Is this the case? If so, could we do something along these lines instead?
const registry = useRegistry();
const actions = [
{
id: 'view',
callback: ( items, { onActionPerformed, callbackContext } ) => {
// onActionPerformed is triggered by DataViews.
// callbackContext is the action's responsibility to define, but DataViews passes it through.
},
callbackContext: {
registry
}
}
];
return <DataViews
actions={ actions }
/>I reckon this is still part of what DataViews has to do (passing the callbackContext to the callback), though the locality makes it easier to reason about (it feels it's part of the Actions API and not of DataViews API) — with the added benefit that each action can have a different context.
There was a problem hiding this comment.
Obviously, it's possible to call the registration action within a hook as well but it does make extensibility a bit harder. When should a third-party script "unregister" a default provided core action.
There was a problem hiding this comment.
What I don't like as well, is that it will actually forces us to retriggers the whole registration action when a selected value used with isElligible is resolved (for instance, when the active rendering mode changes, or active post type, or user permission...)
There was a problem hiding this comment.
I would like actions and fields to be able to be registered a bit more declaratively
Yes this sounds similar to what I also write above: often it's a good idea to define actions and the context separately and independently. It's then up to framework to inject all the dependencies (context) when calling the action.
It's worth exploring passing the context (or context creator function) to DataViews as prop.
Is the context passed to isEligible different from the context passed to the action callback?
There was a problem hiding this comment.
Now, that I think about it more, there's a use-case that can't really be solved with the approach suggested #62505 (comment)
Show/hide an action by checking the permissions for the current item(s). Permissions changes from one row to the other in a data view, so how would you define an action who isEligible call should use a selector value ( canUser ). The only possibility I see is to register/unregister the action when the selection changes. It doesn't seem very ergonomic or DevX friendly to me.
There was a problem hiding this comment.
Not really the same, but a similar sacrifice was made here: #60724
ellatrix
added
Backport to WP 6.6 Beta/RC
Related #61084
Related #55083
What?
This PR adds supports for thunks to the dataviews actions "callback" property. The idea is to avoid using "hooks" to register actions and instead, actions could be top level items just like block types. the callback would receive the "registry" in the callback function.
Testing Instructions
1- Ensure the permanently delete action for posts still works as expected.