marmelab / react-admin

A frontend Framework for single-page applications on top of REST/GraphQL APIs, using TypeScript, React and Material Design
http://marmelab.com/react-admin
MIT License
24.77k stars 5.22k forks source link

Allow a target prop in ReferenceField #2022

Closed christiaanwesterbeek closed 6 years ago

christiaanwesterbeek commented 6 years ago

Is your feature request related to a problem? Please describe. Consider

const CommentShow = props => (
    <Show {...props}>
        <SimpleShowLayout>
            <TextField source="id" />
            <ReferenceField source="post_id" reference="posts">
                <TextField source="title" />
            </ReferenceField>
            <TextField source="author.name" />
            <DateField source="created_at" />
            <TextField source="body" />
        </SimpleShowLayout>
    </Show>
);

In some database all primary keys are named id instead of [table_name]_id. I'm not saying having id as pk's is recommendable, but it is used quite often.

Describe the solution you'd like How about allowing a target in ReferenceField? Just like ReferenceManyField is accepting both source and target.

const CommentShow = props => (
    <Show {...props}>
        <SimpleShowLayout>
            <TextField source="id" />
            <ReferenceField source="post_id" target="id" reference="posts">
                                             ^^^^^^^^^^^
                <TextField source="title" />
            </ReferenceField>
            <TextField source="author.name" />
            <DateField source="created_at" />
            <TextField source="body" />
        </SimpleShowLayout>
    </Show>
);

Describe alternatives you've considered /

Additional context /

fzaninotto commented 6 years ago

React-admin requires that the records it manipulates have an id attribute. If your backend doesn't provide one, it's the job of the dataProvider to do the translation.

Adding the target prop that you ask would imply leaking this "adapter" logic outside of the dataProvider. We won't do it.

christiaanwesterbeek commented 6 years ago

No problem. I hadn't considered translating in the dataProvider. Thanks for the suggestion.

Btw, you guys are awesome! I had this Datagrid with overly nested ReferenceManyFields and ReferenceFields. And it worked flawlessly. Really amazing what you are building! Again, 👏👏👏.

fzaninotto commented 6 years ago

Thanks a lot, we appreciate!

aramando commented 5 years ago

@fzaninotto @aramando I was looking into adding this functionality myself and submitting a PR - I'm glad I checked here first!

But with regard to the reason given here for not supporting a target prop on the ReferenceField component, why does this not also apply to the ReferenceManyField component, which does accept a target prop?

I also don't understand why the requirement for all records to be primarily-keyed by an id property precludes the use case of fetching a record that is related to the current record, just not by its primary key? What if one wishes to reference a related record by a foreign key that exists on the target, e.g. source="id" and target="user_id"?

nickgrealy commented 3 years ago

React-admin requires that the records it manipulates have an id attribute. If your backend doesn't provide one, it's the job of the dataProvider to do the translation.

Adding the target prop that you ask would imply leaking this "adapter" logic outside of the dataProvider. We won't do it.

Sorry about coming in late to the party - one question for @fzaninotto:

If you don't support joins on other properties (i.e. target), then why does ReferenceManyField have a target field? (Are there plans to remove it?)

https://github.com/marmelab/react-admin/blob/1e2e57554b45067a06c4e678b437213d269d25b0/packages/ra-ui-materialui/src/field/ReferenceManyField.tsx#L61-L74

fzaninotto commented 3 years ago

No, there are no plans to remove it from ReferenceManyField.

One of the reasons why ReferenceField requires that you use the id field is that we do an optim in datagrids having one ReferenceField per row to replace many getOne with one getMany... and this only works with the id field.

We don't have (or plan to have) such optims in ReferenceManyField.

lauri865 commented 2 years ago

@fzaninotto, I'm gitting the same constraints as many others, trying to add a ReferenceField to Datagrid in order to pull in data from another table on composite id or just any foreign key that's not id. I'd be most fine adding some translation logic in my dataprovider to do so, but lacking the ability to send any options to the dataprovider from the ReferenceField component.

useGetManyAggregate does pass along meta to the dataprovider, but there's no way to send em through via ReferenceField component + useReference hook. I did an implementation in my codebase, but had to import 400 lines of code just to add 1 prop (=meta) to ReferenceField + the same prop to useReference.

A big win would be the ability to set meta={...} props on ReferenceField that would be sent along to the dataprovider.

fzaninotto commented 2 years ago

If ReferenceField doesn't work in your case, use one of the the data provider hooks (like useGetManyReference together with useList to create a ListContext and use the Datagrid as you like.