React-admin: Add more checking in SimpleShowLayout to only pass props to react-admin components

[RafeSacks]

What you were expecting:
To be able to use non-input components in a <SimpleShowLayout> without prop warnings about "React does not recognize the basePath prop on a DOM element."

What happened instead:
I get warnings when using components like <Typography>.

Steps to reproduce:
Rough example:

    <Edit>
        <SimpleShowLayout>
          <Typography variant="title" gutterBottom>
            My Title
          </Typography>
        </SimpleShowLayout>
    </Edit>

Related code:
I'm fairly new to React still so this may be naive. I used a little workaround to strip props in a div to wrap non-field components, which worked as far as removing warnings, but seems like a workaround rather than a real solutions:

const SanitizeProps = ({ record, resource, basePath, ...rest }) => (
  <div {...rest}>{rest.children}</div>
);

In SimpleShowLayout (https://github.com/marmelab/react-admin/blob/master/packages/ra-ui-materialui/src/detail/SimpleShowLayout.js), I see there is only type checking for strings. I propose only cloning components that need it (fields and inputs?)

[...]
                    ) : typeof field.type === 'string' ? (
                        field
                    ) : (
                        cloneElement(field, {
                            record,
                            resource,
                            basePath,
                        })
                    )}
[...]

Other information:

Environment

• React-admin version: ^2.8.5
• React version: ^16.8.6

[RafeSacks]

At a quick glance it looks like type checking is tricky due to HOCs and such. I really have no idea how best to do this. Maybe adding something to the supported components similar to withStyles innerRef (https://github.com/mui-org/material-ui/issues/10106),

The following pattern seems really fragile but since I pretty much always use the source prop this does work. Again, this doesn't seem like a good solution:

[...]
          ) : typeof field.type === "string" ? (
            field
          ) : field.props.source ? (
            cloneElement(field, {
              record,
              resource,
              basePath
            })
          ) : (
            field
          )}
[...]

[RafeSacks]

Related SO post: https://stackoverflow.com/questions/53278585/custom-layout-in-simpleform-component-on-react-admin

[fzaninotto]

Hi, and thanks for your suggestion.

As much as possible, we try to avoid inspecting children when cloning them. We've found that it creates many problems when developers wrap a react-admin component inside their own component to customize e.g. styles. So props sanitization is the responsibility of the children, and your first approach is correct. We won't add a check to introduce two different ways to pass props to children, that would be fragile and error prone.

So I'm closing this feature request as we won't implement it.

[RafeSacks]

Fair enough, thanks for the reply.

I have to say I'm having a hard time getting used to this part of React where values are not passed explicitly. I'd much rather see all of the props in use and be asked to pass these props to the children explicitly, relying on propTypes isRequired to tell me I am missing interface requirements. I'm sure there are times where directly passing props is needed, but it seems a lot of the time it is just to hide things and make interface seem simpler at a glance.

[fzaninotto]

I can understand your difficulties. I think it might become easier once we migrate to React Hooks - but that will take months.

[boutdemousse]

This should be in the documentation.
I am sure lots of people are facing this problem that can appear in lots of react-admin components.
Like simple things such as putting <h3> in a <FormTab>

If using react-admin means one has to give up on native html tags or wrap them then I think there is something wrong.
(Or maybe remove the _Super easy to extend and override (it's just React components_) from the README)