feat: use storeKey in useRecordSelection

[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.

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! 💪

[slax57]

I'm not sure what @fzaninotto had in mind implied introducing a new fromAllStoreKeys parameters.

My understanding is:

• Calling unselect() or clearSelection() with no storeKey will clear the selection for the default store key (${resource}.selectedIds)
• Calling unselect() or clearSelection() with a storeKey will clear the selection only for that store key (${storeKey}.selectedIds) and that's OK. We agree that it won't clear selection from all store keys because we don't see a use-case where the same items would appear in two list with two storekeys, and even if they do, it should be fairly easy to fix

[slax57]

UPDATE: actually my bad, in all cases when we clear the selection we want to clear the ids from all stored selection states.
So basically this comes down to always having fromAllStoreKeys set to true (but we still don't need the prop).

Also, I better understand now why you had to introduce the notion of namespaces. Otherwise you would have no way to know all the storeKeys from which you need to remove the ids.

However, we'd prefer if you could use the following model instead:

• Keep using one store key per storeKey (i.e. useStore(`${storeKey ?? resource}.selectedIds`, []))
• Add an additional store value to store all storeKeys associated to a resource (e.g. `${resource}.storeKeys` = ['a', 'b']), which will allow you to find all the storeKeys from which to remove the ids when calling unselect() or clearSelection()

I hope my explanations are clear enough. Feel free to ask for more details if it's not the case.

Again, thanks for offering to work on this.

[mikhail-fedosenko]

Thanks. Yes, the idea of having a separate store for listing storeKeys is nice, I'll switch to it...

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.