Introduce separate selection states for lists with distinct storeKey

[mikhail-fedosenko]

Problem

It's not possible to create distinct List views that use separate record selection states.
Previous discussion was started in PR: #10951

Solution

Store selected IDs under in a Record<string, Identifier[]> structure in useRecordSelection. List component passes the storeKey to useRecordSelection. If storeKey is non-empty, then it's used as a key in the selection object. Delete controllers pass the flag to unselect deleted records from all storedKeys.

Reached the following solution after the discussion:

Store the selected IDs as an array in a store, the store key will include passed storeKey.
Also, store the list of used storeKeys per resource in an additional store (1 list of storeKeys per resource).
When the code asks to remove selected IDs it can pass a boolean flag to remove the selection from all store keys, thus, the useRecordSelection will remove selections from all storeKeys stored in that 2nd store.

How To Test

Run the story for record selections states with different storyKeys:
http://localhost:9010/?path=/story/ra-ui-materialui-list-list--record-selection

1. Go to Books list with storeKey A; select some records.
2. Go to Books list with storeKey B; select some records.
3. Go back to list with storeKey A - check selected records (they should persist from step 1).
4. Go to Books list view without storeKey; select a single book that's selected on previous lists; Delete it. Check that this book was unselected from all lists (selection counter decremented).

Additional Checks

• The PR targets master for a bugfix or a documentation fix, or next for a feature
• The PR includes unit tests (if not possible, describe why)
• The PR includes one or several stories (if not possible, describe why)
• The documentation is up to date

[mikhail-fedosenko]

This is just a draft to make sure the solution is applicable for the framework.
I've tried to keep backwards compatibility (selection state without namespace is stored under same key in localstorage). With major release we can migrate to new datatype in the store, so it will be a breaking change (for anyone reading and writing directly to the store, bypassing the useRecordSelection), but only one key will be used though.

Btw, is there any chance to get this feature in minor release?

[fzaninotto]

can you explain why you need to introduce the notion of namespace, which seems to overlap with the storeKey feature?

[mikhail-fedosenko]

It's maybe more like storeKey, i just named it like this...
I want to allow the user to select different sets of records on different views: for example, our system has members, members belong to teams. When a user goes to Team A he can select some members, when viewing the Team B, ideally, he should have another selection state.
Since the List view's storeKey doesn't affect selection state (it's stored "globally" per resource) I've introduced the storeKey/namespace for selection state.

Do you suggest to pass List's storeKey to useRecordSelection instead of adding separate & independent key for selection state?

[fzaninotto]

Do you suggest to pass List's storeKey to useRecordSelection instead of adding separate & independent key for selection state?

Exactly

[mikhail-fedosenko]

@fzaninotto ok, will do.
So, it will be a breaking change. Since new features are usually targeted to next, does it mean it will appear in next major release if I finally prepare the PR? :)

[fzaninotto]

Why will it be a breaking change? You only need to make the unselect() affect all storekeys by default.

[mikhail-fedosenko]

Any existing usage of List with non-empty storeKey will start using separate selection state.

[fzaninotto]

So you're right, it's a breaking change, but one that we're willing to accept given =i see no use case for two lists with different store keys that would require an identical selection.

You can continue on next and we'll consider your PR for a future minor release.

[mikhail-fedosenko]

Ok.

What about storing selection state in 2 separate keys (${resource}.selectedIds + ${resource}.selectedIds.namespaced) VS storing the selection state as new type in the existing key (${resource}.selectedIds)?
In current PR's state it stores "global" state under old key to keep backwards compatibility for those, who read the (local)storage directly, bypassing the useRecordSelection.
I would prefer simpler solution (store new type in the existing key), since I personally don't expect anyone (or at least the substantial number of people) to use direct access to selected IDs in the storage...

@fzaninotto , can you please have a look at the question above?

[mikhail-fedosenko]

@fzaninotto as far as I can understand the requirements for the PR it seems to be ready for the review.

[slax57]

Sorry we're taking some time to review your PR but the team is fairly busy ATM 😅 .
Still, we appreciate your work and your will to contribute! 💪

[mikhail-fedosenko]

@slax57 , i've pushed my changes to resolve your comment about keeping the list of storeKeys in a separate store.

Regarding the fromAllStoreKeys parameter, copying my explanation from above:

Regarding introducing fromAllStoreKeys - it seemed to me that removing selected IDs from all storeKeys by default may be not intuitive (e.g. unexpected side-effects) for the users of useRecordSelection hook. From the perspective of existing code in React-Admin (delete cases) - well, yes, this argument is always true.
In other words, IMO, unselecting usually will happen in scope of current storeKey (fromAllStoreKeys === false), but deletion case is more specific and requires clearing from all storeKeys... I could have added a new method for this VS adding a parameter, but have chosen to keep the API surface smaller and be able to force unselection everywhere for the delete case.

I'm ready to remove it and always delete from all store keys if we want to use this behaviour as a default one (w/o ability to customize the behaviour from the code outside RA).

[slax57]

@mikhail-fedosenko Thank you for working on that new iteration.

I re-read the whole discussion, and in the end, I think I agree with you.
The behavior that probably makes most sense is to have each selection state be independent from one another by default (i.e. have fromAllStoreKeys set to false by default).
The only exception being the case when the records are deleted, in which case it makes sense to remove their ids from all selection states tied to this resource (i.e. have fromAllStoreKeys set to true for that particular use case).

I need to re-discuss this with @fzaninotto, we'll be in touch.

The CI tests found a breaking change (see below), and I also found some minor issues, but other than that you code looks good to me!

Thanks again!

[slax57]

UPDATE: @fzaninotto also agrees with your POV. So I confirm you can keep the fromAllStoreKeys param.
Thanks!

[mikhail-fedosenko]

@slax57 , updated the docs, reverted the BC in useUnselectAll, added fromAllStoreKeys as the 2nd optional argument in useUnselect.

If you'd like to have more detailed examples in the docs about new fromAllStoreKeys, I'm ready to add them..

[slax57]

Thanks!