Actually, clientId can be destructured, too.

I really like the main refactoring of moving this handling to the individual block item, but I'm not sure how I feel about this specific change of calling a selector within the component itself. I don't think there's anything bad about it in the sense of generating inaccurate data, but it does feel like something which would be easy to overlook as far as the general pattern of providing data to components via a selector vs. passing the selector itself. That said, passing the selector does provide opportunities to optimize when the data is only needed in very specific circumstances like this.

Alternatively, I'm left to wonder if it's still something we could handle in the withDispatch callback, receiving the selection start from withSelect and somehow blocking it from reaching the component. I don't know that this is easily achievable without something like mapProps (to drop the unwanted prop) and a separate layer of pure. Not only is this pretty cumbersome, it probably has worse performance than if we were to just pass the data anyways.

A variation to this would be to still have the bulk of the handling occur in withDispatch, but call select to retrieve the value from getBlockSelectionStart. It's not too much different than what we're doing here, so I don't know that it really is much better.

What do you think?

You had even described it as confusing when @iseulde had proposed the same thing in #10431 😆

#10431 (comment)

You had even described it as confusing when @iseulde had proposed the same thing in #10431

Yes, I remember that. I still think that it is an issue. That's why I added an inline comment :)

A variation to this would be to still have the bulk of the handling occur in withDispatch, but call select to retrieve the value from getBlockSelectionStart. It's not too much different than what we're doing here, so I don't know that it really is much better.

I think the easiest change would be to add select as 3rd param to withDispatch HOC, which should work out of the box with the current implementation and would allow retrieving the value which causes this discussion inside the event handler without exposing it as a prop. Now, the question is if we want to confuse plugin developers with this new capability. I really like how symmetric and straightforward are withSelect and withDispatch as of today.

Alternatively, I'm left to wonder if it's still something we could handle in the withDispatch callback, receiving the selection start from withSelect and somehow blocking it from reaching the component. I don't know that this is easily achievable without something like mapProps (to drop the unwanted prop) and a separate layer of pure. Not only is this pretty cumbersome, it probably has worse performance than if we were to just pass the data anyways.

Redux solves this issue by offering 3rd param called mergeProps inside connect HOC. However, it's very difficult to use and even more difficult to read. Besides connect is one HOC so you can avoid re-renders by filtering unwanted props, but the cost is that you have to perform lots of computations to upfront to make it work.

It would be great if we would come up with a pattern to ensure we have a common way of handling similar use cases. In general, I observe that there are many places where we compute props which are never used for rendering but they sit there and wait until a given event occurs.

We can just pass select as a third argument in withDispatch for these advanced use-cases.

Edit I just saw that it was already proposed :)

I don't really like the idea, partly because with how proxying works, we shouldn't imply that withDispatch can be a method for injecting up-to-date state values to a component. In the very worst case, someone could even wrongly think to ignore withSelect altogether and just use withDispatch for all their needs.

I'm thinking more that wp.data.select within the withDispatch callback is maybe sensible, as its intended usage is for exactly these moments of one-off data retrieval.

Is the rerendering here a big issue to sacrifice devx like we're trying to do by finding solutions here? Maybe the current approach by @gziolo in the PR is just fine as it's well commented as a performance optimization. I'm thinking the generalization might not be worth it because it doesn't seem so common.

I don't think there is an easy solution, everything we do have some drawbacks. If you add 3rd params select to withDispatch it might be used incorrectly. Example: when you call selector outside of the event handler, it might not re-evaluate properly when the value you want to defer changes because it isn't reflected in props. So you still need to make sure it's called inside the handler.

Is the rerendering here a big issue to sacrifice devx like we're trying to do by finding solutions here?

Yes, I think so. More-so being that this is BlockListBlock.

I see it as something we should consider having some guidelines around, since it's not the first time this has come up.

What's the drawback with calling the main select (the one exported by @wordpress/data) from the withDispatch callback?

Yes, I think so. More-so being that this is BlockListBlock.

Agreed with this, that we should make this particular component performant (and the current approach is also sacrificing good code, because passing selector is not great either but it's localized)

What I'm not certain about is the changes we need to do in the data module to accommodate this.

This could just be onMultiSelect: multiSelect