Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

storeKey prop is not passed to useRecordSelection hook inside useListController #9740

Closed
nbalaguer opened this issue Mar 22, 2024 · 5 comments
Labels

Comments

@nbalaguer
Copy link
Contributor

nbalaguer commented Mar 22, 2024

English is not my native language, my apologies in advance for any weird grammar or implicit tone of the writing. I'll try my best 😃

What you were expecting:

I would expect the storeKey prop to work on both useListParams and useRecordSelction hooks.

What happened instead:

Right now the storeKey prop only gets passed to the useListParams hook.

Steps to reproduce:

Related code:

Prop is received

const {
debounce = 500,
disableAuthentication,
disableSyncWithLocation,
exporter = defaultExporter,
filter,
filterDefaultValues,
perPage = 10,
queryOptions = {},
sort = defaultSort,
storeKey,
} = props;

Prop is forwared to useListParams

const [query, queryModifiers] = useListParams({
debounce,
disableSyncWithLocation,
filterDefaultValues,
perPage,
resource,
sort,
storeKey,
});

Prop is NOT forwarded to useRecordSelection

const [selectedIds, selectionModifiers] = useRecordSelection(resource);

storeKey is hardcoded based on the resource inside useRecordSelection

const storeKey = `${resource}.selectedIds`;

useListParams uses state to keep localParams when storeKey is false and the custom storeKey if provided, defaulting to a storeKey derived from the resource. useRecordSelection should do something similar.

Other information:

In this example, using 2 lists to display the same resource, one with storeKey set to false and the other with storeKey set to a custom string, you can see that when selecting a row in one list, the same row gets selected in the other list.

https://stackblitz.com/edit/github-asfjgd?file=src%2Fposts%2FPostList.tsx

Environment

  • React-admin version: 4.16.12
  • Last version that did not exhibit the issue (if applicable): -
  • React version: 18.2.0
  • Browser:
  • Stack trace (in case of a JS error):
@djhi
Copy link
Collaborator

djhi commented Mar 22, 2024

Good catch! Would you mind opening a PR to fix it?

@djhi djhi added bug wontfix and removed bug labels Mar 22, 2024
@djhi
Copy link
Collaborator

djhi commented Mar 22, 2024

Actually, we already had a few similar issues about this and the answer is not so simple: see #8536 and #7401

@nbalaguer
Copy link
Contributor Author

@djhi

Hi there, thx for the quick reply! I've thought for a while about the issue at hand and read through the linked related issues and wanted to add my two cents to it.

First of all let me restate the issue to know if I undertood it well enough:

When implementing row selection in lists, resource-bound global selection state was chosen for, among others, one main goal: keep selection accross navigations (mainly list view <-> edit view back and forth user experience) and be able to delete records from other views (mainly the edit view) and being able to deselect (if selected) that record so when the user goes back to the list all is consistent.

Since then, a custom storeKey prop has been added to the list controller to allow different lists using the same resource to be, for example, filtered independently from one another.

This independence cannot be applied to the selection state because it would break the stated main goal, since when deleting a record from its edit view there could be arbitrary keys in the store that would have to be updated, which is not possible or very hard to manage.

Now with the problem stated, I think that the custom storeKey funcionality is not 100% complete, let me explain:

I would argue that the meaning of storeKey={false} is "For this list, I don't want any store interaction", not for list params and also not for selected rows, no interaction at all. For this reason, and only if storeKey is false, the list should keep the selected rows in local state, and use the resource-derived storeKey otherwise. That would mean that if I navigate away and come back, the list won't have any selected rows, but I chose that consciously.

By using "the resource-derived storeKey otherwise", we avoid breaking react-admin's desired interactions with the store for selection related stuff. For normally managed lists, current behaviour is kept in all cases. For storeKey={false} lists, any navigation away from the page will loose (unselect) its selection state, so deletions from another page don't have to worry about ghost selected rows for that list.

I've made a quick PR so we have some code to look at, and also to show that the changes seem to be limited to a couple files.

@fzaninotto
Copy link
Member

@nbalaguer that seems like a sensible approach, thanks for taking the time to explain it.

@fzaninotto
Copy link
Member

Fixed by #9742

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants