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

[RFR] Add option to make ReferenceInput choices lazy #6013

Merged
merged 10 commits into from
Oct 5, 2021

Conversation

ValentinH
Copy link
Contributor

@ValentinH ValentinH commented Mar 8, 2021

This is actually implementing a similar feature than the one requested in #5898 but for <ReferenceInput.

Why

The goal is to be able to control when the useGetList is enabled to avoid over-fetching the choices. In our application, we have a user autocomplete component and, as we have hundred of thousands of possible choices, we only want to start showing/fetching the choices if the user inputs at least 2 characters.

Solution

This PR introduces an isEnabled option that is an optional function we use to set the enabled option of the useGetList(). This function is passed the filterValues array.

Example of usage:

<ReferenceInput
  label="example"
  reference="users"
  source="user"
  isEnabled={({ q }) => {
    return !!q && q.length > 2
  }}
>
  <AutocompleteInput ... />
</ReferenceInput>

Alternatives

Today, in order to work-around this, we pass a permanent filter filter={{ is_autocomplete: true }} that we use on our backend api to return early to avoid hitting the DB. However, we are still making calls for nothing.


I also looked to add a similar prop for the ReferenceArrayInput but, as it's using useGetMatching, it's a bit more complex.

If you think this option makes sense, I'd be happy to dig more to also add support for it for the ReferenceArrayInput.

Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Nice! This will be a welcome addition.

You need to add unit tests and documentation to have this PR ready to merge.

} = useGetList(reference, pagination, sort, filterValues);
} = useGetList(reference, pagination, sort, filterValues, {
action: 'CUSTOM_QUERY',
enabled: isEnabled && isEnabled(filterValues),
Copy link
Member

Choose a reason for hiding this comment

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

This won't work when isEnabled isn't set

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 actually relied on the enabled option default value being true. However, I can make it true explicitly if you think this is clearer.

@ValentinH
Copy link
Contributor Author

OK, happy that you like it. I'll add the tests and docs then.

What do you think about my comments regarding ReferenceArrayInput ?

@ValentinH ValentinH changed the title [RFC] Add option to make ReferenceInput choices lazy [RFR] Add option to make ReferenceInput choices lazy Mar 11, 2021
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

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

Nice! Feel free to do the same for ReferencearrayInput, too.

docs/Inputs.md Outdated Show resolved Hide resolved
@ValentinH
Copy link
Contributor Author

ValentinH commented Mar 12, 2021

From what I see useReferenceArrayInputController() uses useGetMatching() and the enabled option is actually not working properly there: when the enabled flag is false, the loading prop is stuck to true which will show the loading indicator. I need to dig more into this and probably fix the enabled prop behaviour for useGetMatching.

If you agree, I'll do this in a follow-up PR.

Edit: I've a working solution, I'll push it later today 🙂

@ValentinH
Copy link
Contributor Author

I just pushed the support for the enableGetChoices prop on the ReferenceArrayInput.

One small thing that I find misleading is that the filterValues always contain the q param for ReferenceInput but not for ReferenceArrayInput. This enforces checking for q being present in the ReferenceArrayInput enableGetChoices function:

image

Do you think this is something that should be taken care of or is fine like this?

@ValentinH
Copy link
Contributor Author

@fzaninotto do you think this PR is ready to be merged or is it still missing something?

@fzaninotto fzaninotto closed this Jul 8, 2021
@fzaninotto fzaninotto deleted the branch marmelab:master July 8, 2021 16:04
@ValentinH
Copy link
Contributor Author

Why is this being closed?

@fzaninotto
Copy link
Member

This PR was automatically closed by GitHub, I'm reopening it. Sorry for the noise.

@fzaninotto fzaninotto reopened this Jul 8, 2021
@fzaninotto fzaninotto deleted the branch marmelab:master September 7, 2021 09:55
@fzaninotto fzaninotto closed this Sep 7, 2021
@ValentinH
Copy link
Contributor Author

I think this PR was closed again by Github. I believe it is still valid.

@fzaninotto
Copy link
Member

You're right, I'm reopening it.

@fzaninotto fzaninotto reopened this Sep 7, 2021
@djhi djhi deleted the branch marmelab:master October 4, 2021 09:03
@djhi djhi closed this Oct 4, 2021
@djhi djhi reopened this Oct 4, 2021
@dgkm
Copy link

dgkm commented Oct 4, 2021

It's a great feature. Any idea in which release this PR is getting merged and will be available?

@djhi djhi changed the base branch from next to master October 5, 2021 07:28
@djhi djhi added this to the 3.19 milestone Oct 5, 2021
@djhi djhi dismissed fzaninotto’s stale review October 5, 2021 07:30

Requested changes have been applied

@djhi djhi merged commit dc5f6ab into marmelab:master Oct 5, 2021
@ValentinH
Copy link
Contributor Author

Thanks for merging this PR! 🎉

@ValentinH ValentinH deleted the reference-input-is-enabled branch October 5, 2021 08:19
@djhi
Copy link
Collaborator

djhi commented Oct 5, 2021

Thanks!

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.

4 participants