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] Introduce useGetManyReference hook #3697

Merged
merged 4 commits into from
Sep 16, 2019
Merged

Conversation

djhi
Copy link
Contributor

@djhi djhi commented Sep 16, 2019

No description provided.

@djhi djhi added this to the 3.0.0 milestone Sep 16, 2019
pagination: Pagination,
sort: Sort,
filter: object,
source: string,
Copy link
Member

Choose a reason for hiding this comment

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

unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used: sent in the payload just like the old call to crudGetManyReference in useReferenceManyFieldController did. See here

Copy link
Member

Choose a reason for hiding this comment

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

Then I don't understand what it does. And the dataProvider doc doesn't mention it, so I doubt anyone uses it.

* };
*/
const useGetManyReference = (
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.

I don't think this should be the first argument, because it's not actually used for the query - just for the storage in the Redux store.

Suggested signature: ('comments', 'post_id', post_id, { page: 1, perPage: 10 }, { field: 'published_at', order: 'DESC' }, 'posts')

And this last argument should probably be called something like "cache_key"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only a part of the cache key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Besides, I thought it would be better to have the same arguments order as in the other dataProvider hooks

Copy link
Member

Choose a reason for hiding this comment

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

I think it's confusing, as the actual resource that this hook fetches is the reference. If you think about this hook outside of the context of the ReferenceInputs, it's just a way to get a list of resources wit ha particular filter...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const useGetManyReference = (
    reference: string,
    target: string,
    id: Identifier,
    pagination: Pagination,
    sort: Sort,
    filter: object,
    referencingResource: string,
    options?: any
) => { ... }

?

packages/ra-core/src/dataProvider/useGetManyReference.ts Outdated Show resolved Hide resolved
@fzaninotto fzaninotto merged commit 4717447 into next Sep 16, 2019
@fzaninotto fzaninotto deleted the use-many-reference branch September 16, 2019 15:58
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