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

Disable all store interactions for a list with storeKey set to false (sort, pagination, filters and now also selection state). #9742

Conversation

nbalaguer
Copy link
Contributor

Proposed update in #9740

@fzaninotto
Copy link
Member

Nice! Would you mind adding a unit tests for the local version?

*
* @returns {Object} Destructure as [selectedIds, { select, toggle, clearSelection }].
*/
export const useRecordSelection = <RecordType extends RaRecord = any>(
resource: string
resource: string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if disableSyncWithStore is true, resource isn't required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the unit tests based on the already implemented ones, let me know if anything needs to change!

Regarding the typing of the hook I have some doubts:

Would it be enough to just set resource to optional? I'm not really sure since it may not error when calling this hook without any arguments.

After searching around to find information about making function parameters optional based on other parameters and messing around with the typescript playground, I've arrived to this type definition. I didn't want to change the hook from an arrow function to a normal function and this is the best I've been able to come up with (after a fair bit of googling) to do some kind of signature overloading.

In the react-admin project, since strictNullChecks is not enabled, lines 12 and 13 of the playground code do not error.

What would be the best way to approach this?

Copy link
Member

@fzaninotto fzaninotto Mar 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change the argument to an object:

type UseRecordSelectionWithResourceArgs = {
 resource: string;
}
type UseRecordSelectionWithNoStoreArgs = {
  disableSyncWithStore: false;
}
type UseRecordSelectionArgs = UseRecordSelectionWithResourceArgs | UseRecordSelectionWithNoStoreArgs;
export const useRecordSelection = (args: UseRecordSelectionArgs) => {
  // ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And then change all instances of calling the hook to use an object right? If that's ok I'll get to it today :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! But again this must be done in the next branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! I hope this is not too much of a bother but to avoid any issues when changing the destination branch to next (there were a couple of commits that weren't mine) I updated my branch by starting directly from the latest state of next and applying my changes in a single commit.

The finishing changes are:

Changing useRecordSelection's signature

Following your suggestion, I changed from (resource, disableSyncWithStore) to (args). I had a couple of issues with only having

type UseRecordSelectionWithResourceArgs = {
 resource: string;
}
type UseRecordSelectionWithNoStoreArgs = {
  disableSyncWithStore: true;
}

as the type. When using the hook like

const [selectedIds, selectionModifiers] = useRecordSelection({
    resource,
    disableSyncWithStore: storeKey === false,
});

typescript was complaining that boolean is not assignable to true, so I modified it a bit and arrived to

type UseRecordSelectionWithResourceArgs = {
    resource: string;
    disableSyncWithStore?: false;
};
type UseRecordSelectionWithNoStoreArgs = {
    resource?: string;
    disableSyncWithStore: true;
};

which seems to work well. The error disappears and it still complains if you don't specify a resource and don't set disableSyncWithStore to true.

Updating the documentation

I also tweaked a bit the documentation when talking about storeKey stuff. I hope it reflects the changes made to the use of storeKey apropriately. If any improvements need to be made plese feel free to tell me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fzaninotto I realized my email address was wrong inside my linux subsystem and was authoring commits from a different account to this one. I updated it that's why the commit is so recent. Sorry for any inconvenience :(

@fzaninotto
Copy link
Member

Also, this must be PRed against next as the master branch is feature-freeze.

@nbalaguer nbalaguer changed the base branch from master to next March 26, 2024 17:39
@nbalaguer nbalaguer force-pushed the fix-storeKey-not-being-used-by-useRecordSelection-hook branch from 5f94b59 to c34be8d Compare March 27, 2024 01:04
@nbalaguer nbalaguer changed the title Setting storeKey to false opts out of all store interactions (params and selection state) for that list. Disable all store interactions for a list with storeKey set to false (sort, pagination, filters and now also selection state). Mar 27, 2024
…(sort, pagination, filters and now also selection state).
@fzaninotto
Copy link
Member

The result is great. Thanks a lot for your work!

@fzaninotto fzaninotto merged commit d6c183a into marmelab:next Apr 3, 2024
11 checks passed
@fzaninotto fzaninotto added this to the 5.0.0 milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants