React-admin: [RFC] Replace injected elements by injected components

[fzaninotto]

In React parlance, an element is an object that becomes part of the virtual DOM, and a component is a function (or a class) that renders an element.

// that's a component
const Layout = props => <div ... />

// that's an element
<Layout />

Current situation

In react-admin, many components accept elements as props, like for instance List:

const MyList = props => <List {...props} pagination={<MyPagination />} />

It's quite unusual in the react world. Most JSX libraries that do some kind of dependency injection use components for that, like for instance material-ui:

<Avatar component={MyComponent} />

The reason to use elements in the first place was to allow for injection of dynamic elements, i.e. depending on the current props.

const MyList = ({ resource, ... props }) => (
    <List {...props} resource={resource} pagination={<MyPagination perPage={resource== "posts" ? 10 : 25} />} />
);

Also, the children props accepts elements, not components. Allowing the injection of elements in other props seemed like a logical extension to the React convention.

Problem

However, in some places in the code, we do have to accept components because that's what our dependencies expect. Like for instance in Resource:

<Resource name="posts" list={PostList} />

The result is confusion: react-admin users never know when they must inject components, and when they must inject elements.

Another problem with element dependencies is runtime and compile time props validation. For instance, even though the Pagination component cannot work without a page and a total prop, we cannot mark these props as required because we must accept that injected as <Pagination /> by developers (and then, the List component injects the page and total props).

Proposal

So my proposal is: let's move all injections in react-admin to components.

// replace
const MyList = props => <List {...props} pagination={<MyPagination />} />;

// with
const MyList = props => <List {...props} pagination={MyPagination} />;

That's a major BC break for sure, but I feel like the upgrade task isn't that hard (it's a matter of looking for the {< string and replacing it smartly everywhere).

Besides, the motivation for accepting elements (having a dependency configured via props) is still possible if we inject components. It just has to be a dynamic component:

const MyList = ({ resource, ... props }) => (
    <List {...props} resource={resource} pagination={p => <MyPagination {...p} perPage={resource== "posts" ? 10 : 25} />} />
);

And, from my point of view, configuring dependencies based on props is more the exception than the rule.

You may wonder: but then, how about the children prop? Should it still accept an element? The answer is yes, because that's the default behavior in react.

Feel free to voice your opinion.

pros

• less confusion
• consistent with other libs
• better props validation
• shorter to write

cons

• breaking change
• a bit harder to inject dynamic dependency

[cherniavskii]

I agree, this would be less confusing!

[deksden]

Small note: List tag in example with MyPagination in "Proposal" section is not closed. Maybe also some missing closing code (curly brace, etc).

[fzaninotto]

@deksden thanks, fixed

[fzaninotto]

Fixed by #3262

[fzaninotto]

I think we went too fast on this one. One of my argument is partly wrong:

It's quite unusual in the react world. Most JSX libraries that do some kind of dependency injection use components for that, like for instance material-ui

Material-ui sometimes expects elements, sometimes components. In fact, it expects elements more often than components. There isn't any rationale in that choice.

React itself accepts elements in the <Suspense fallback> prop.

So there is no consensus as to whether injections should take the form of elements and components.

Also, after migrating our injections, there are still some props that we pass to other libraries - like material-ui for instance. Some of these props are still elements. So even after making our codebase consistent, the userland code still features both element and component injections. The benefit of less confusion isn't obvious.

Besides, #3262 shows that the migration will be very painful - it took @djhi about a day to migrate the react-admin code. I think the cost will be the same for a moderate size project. So the first Con is a big con.

So I'm thinking about 3 possible paths from here:

1. Do the migration anyway (it's already been done actually). That means we'll probably lose many users in the migration process as it'll be painful, but we'll have a clean(er) API.
2. Revert the migration. We can do it in the future, when the React community agrees on a standard way to inject dependencies. People will have to live with the inconsistencies in react-admin, but after all nobody ever complained about it until now.
3. Make react-admin accept both elements and components, and document only components. That way we have a clean codebase, without forcing a migration. This can be done by a helper function, which will throw a deprecation warning a bit before the next major release.

I'll think about it a bit more before making a decision. Comments are welcome.

[Bnaya]

A very welcome change :)
It is also much easier to write with typescript, and have type safety for the components props!

[fzaninotto]

I have witnessed another side effect that is a strong con for that change: injecting a dynamic component has an unexpected behavior, e.g:

const MyList = ({ resource, ... props }) => (
    <List
        {...props}
        resource={resource}
        pagination={p => <MyPagination {...p} perPage={resource== "posts" ? 10 : 25} />}
    />
);

In that example, the pagination component is a new one every time MyList rerenders. As a consequence, updating MyList will mount then unmount the MyPagination component, and not update it.

This does not happen if I inject an element, e.g.:

const MyList = ({ resource, ... props }) => (
    <List
        {...props}
        resource={resource}
        pagination={<MyPagination {...p} perPage={resource== "posts" ? 10 : 25} />}
    />
);

See https://codesandbox.io/s/nifty-burnell-wybop for a test.

This is a gotcha that few developers will expect. The workaround is to wrap the inline component with a React.useMemo(), and it's not available in class components... So not only does it make the migration harder, it introduces edge cases that are hard to spot and cumbersome to fix.

In my opinion, this is enough to Revert the migration (proposition 2 in my previous comment).

[Bnaya]

There are few other ways to tackle it:

First, lets clarify the issue:

pagination={p => <MyPagination {...p} perPage={resource== "posts" ? 10 : 25} />}

What we have here is a function declaration inside JSX.
Declaring a function inside JSX, is considered a pitfall, but sometimes, the syntax is just too nice.
If its just a callback, you just get react update every time (violating immutability)
But it we take that function as a component, React will remount it every time, as the identity of the function have changed.

What can we do to not make developers fall into it?

Educate the developers to not declare the function inside the JSX. if you pass pre-declared class or function, you won't have any issue. We don't have to use React.useMemo.

Or

We can actually make pagination not to take a component, but only a regular function that takes parameters, and inside the component call it like so:

function List({props:{paginationRender: (paginationParams: IpaginationParams) => {}}) {
<div>{props.paginationRender()}</div>
}

And NOT:

function List(props :{paginationRender: (paginationParams: IpaginationParams) => {}}) {
<div><props.paginationRender /></div>
}

For example: React Router route component:
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/modules/Route.js#L68

you have prop for component, and a prop named render.

If you can actually pass the same inline function to component or render,
If you will pass it to component, you will get remount, and if to the render - you will not.

[djhi]

@Bnaya Thanks for your explanation and react-router sample. I can confirm what you say.
@fzaninotto See this fork of your sandbox: https://codesandbox.io/s/currying-night-2ryp2

Using only render props, we could keep the nice syntax and avoid the TS issues you mentioned before

[Bnaya]

Thank you for this amazing library:)

There are some implications when calling user function as part of the render or the lib render,
For example, the developer can try to use hooks directly there (and not just return jsx)
And then the hook counts as part of our component, or fails, if we use class component.
And can make none-clear react stack traces

If we want to bulletproof the lib,
We can take the jsx we got from the renderprop and wrap it in a component that all it do is to render it.
I can write a code for that when ill be on my laptop

[Bnaya]

For render prop isolation see:
https://codesandbox.io/s/shy-leaf-sc2sl

[fzaninotto]

Changing from injected elements to injected render props won't bring us any of the pros I listed in the issue description. It only forces users to do a painful migration with no added benefit IMO.

[djhi]

It would resolves the TS issue though and the props discoverability as you'll receive them explicitly, no?

[Bnaya]

Changing from injected elements to injected render props won't bring us any of the pros I listed in the issue description

I'm not sure if i follow.

I i will take your pitfall example:

const MyList = ({ resource, ... props }) => (
    <List
        {...props}
        resource={resource}
        pagination={p => <MyPagination {...p} perPage={resource== "posts" ? 10 : 25} />}
    />
);

If inside List component,, we take pagination and use it like: pagination(paginationParams) and not <pagination {...paginationParams} />

Or i'm missing something?

We can also take React Router approach, and have both component and render.
If the developer will miss-use the component, and pass inline function, well we are in the same boat of React Router. (I can find additional examples if it will help to make a decision)

[fzaninotto]

This RFC is supposed to solve a problem. The pros to using component injection, that I listed in the description, are:

1. less confusion
2. consistent with other libs
3. better props validation
4. shorter to write

I think render props only help with item 3, to some extent.

But the price to pay (migrate existing code using element injection to code using render prop) is high, because react-admin uses injection a lot.

So my point is, using render props, the cost / benefit ratio for the end user isn't good enough to justify the change.

[fzaninotto]

Thanks you all for your comments.

My decision is made, we will NOT implement that RFC. The job already done in #3262 will be reverted in #3312.