"render props" and "function children" are different; no matter how they're implemented, children aren't just a normal prop - they're special (the third argument to createElement wins over a "children" prop, for example).

Please don't encourage function children.

Please don't encourage function children.

@ljharb Can you explain why this shouldn't be encouraged?

@paularmstrong One argument would be that children is almost always used as an implicit prop, but children as a function encourages using children as an explicit prop, which will lead to a bugs when someone attempts to use both at the same time.

<Thing children={firstFn}>
  woops
</Thing>

React.createElement(Thing, { children: firstFn}, 'woops')

{
  type: Thing,
  props: { children: 'woops' },
  ...
}

Yes, that can already happen, but I've never seen anyone write something like this:

<Thing
  children={
    <Well>
      <This is='awkward' />
    </Well>
  }
>
  ...
</Thing>

FWIW, @ryanflorence convinced me that it's easier to teach people a render prop accepts a function than it is to teach folks that the children prop can be a function. So in my teaching and open source I've been moving things over to render exclusively to hopefully make things more cohesive across the community while making it easier for people to pick up.

So, I'm in favor of using a prop called render personally.

I completely agree with using render prop instead of children. In any instruction/demos I have given it is much easier for people to wrap their head around a prop called render over children being a function.

That just depends on how you format this. You don’t have to indent it an extra time.

<MyComponent>
  {x => (
    <div>{x}</div>
  )}
</MyComponent>

That's how Prettier formats it too.

By default, Prettier formats both as 1-liners 😄

<div>
  <MyComponent>{x => <div>{x}</div>}</MyComponent>
  <MyComponent render={x => <div>{x}</div>} />
</div>;

Just my 2 cents, if we go with a render prop we may as well drop children all together, no reason to keep two props that serve the exact same purpose...

Just to say, please keep children, if one prefers the explicit prop approach, they can do:

<Foo children={x => x} />

If one does:

<Foo children={x => x}>y</Foo>

I'd first ask myself what am I trying to achieve, and then think

the inner content of a component is just treated as children, so I'm just defining children twice, where the latter will take precedence, like any other prop in React

I just want to support the part where @FezVrasta wrote "please keep children-as-func". :)

@FezVrasta I also mention it here: https://github.com/MailOnline/libreact/blob/master/docs/en/Introduction.md#facc

If they have a function name, they don't need a displayName - perhaps making getComponentName(…) required, but not displayName specifically?

My original point is that with this API (unlike existing one) we don’t know which context is missing to show a meaningful warning. Passing a name (explicitly or implicitly) to createContext would solve that.

Totally fair, I'm 100% fine with requiring a name - just not requiring displayName specifically, since normal function names should be more than sufficient.

createContext() isn’t bound to any component or function in particular so it can’t read a .name from it. Therefore my proposal is to make it an argument (even if optional). I think that’s what displayName was referring to. For example:

const Theme = React.createContext('light', {
  displayName: 'Theme’
});

Sorry if I’m missing your point.

Another option is

const Theme = React.createContext('light');
Theme.displayName = 'Theme';

@gaearon ah, I misunderstood. In that case, yes, I'd say requiring displayName makes perfect sense.

Static patterns like:

const Theme = React.createContext('light');
Theme.displayName = 'Theme';

can often prevent tree-shaking/dead code elimination. I'd advise not to recommend this officially, better to have an optional extra param for createContext + maybe auto add static displayName with babel plugin for the development purposes.

named prop, please. children are special, and should only be nodes.

+1 for render

children are special, and should only be nodes.

Would you mind elaborating on this a little more?

One thought I have is that a new prop could be considered as API bloat if children already functions exactly as it needs to. I'm not convinced one way or the other on this one, by the way; I'm just sharing one more thing worth considering.

The type signature for the children prop is pretty consistent across all built-in React components (that I can think of) and most components shipped with third party libraries - Component | SFC | string | etc. The two big exceptions that I've seen are components that don't support children at all and components that require a single child component (React Router's FooRouter components spring to mind).

In particular I think the distinction between (foo) => <DomElement /> vs ({foo}) => <DomElement /> and {renderProp} vs <FunctionalComponent /> is a fine one. I could see that getting confusing in particular if the render prop function isn't defined inline.

All that is why I prefer an explicit 'render' prop, anyway. My knowledge of the React ecosystem is not nearly as in-depth as many of yours' - are there lots of big exceptions that I'm not thinking of?

Edit to add: this is all a user's perspective, not a package author's. If you're still gonna try and discourage users from using the new Context API all of the above might be considered a plus.

That makes sense to me, @JNaftali . I’m now leaning more in favor of a separate prop as well.

You can always pass children as prop:

<Foo children={() => {...}} />

@ljharb Components have always been free to do whatever they want with children. I would agree with you if React by design always passed through children unaltered, but there are so many library components out there that conditionally render children, render them inside wrappers (e.g. interactive rearrangeable grids), etc. that I think "children are special" is a dangerous argument that leads to restricting the power of React.

also, saying that children should only be nodes because they are "special", or because the third argument to createElement wins over a "children" prop, are non-arguments. They don't really explain what if anything you consider cleaner, easier to read, easier to understand, less error prone, etc. about ensuring that children are only nodes.

I don't see how a convention could possibly 'restrict the power of React'. Conventions don't stop you from doing anything. They're just a tool to make it easier for other people to understand how to use the things you write. React's power is such that given any one of these APIs it'd be easy to implement any of the others that will remain true no matter which of these APIs get implemented.

That's true, it's not clear to me if @ljharb is just advocating this as a convention or if he would be in favor of dropping support for function children from React.

As far as making things easier to understand...personally I find child functions more obvious than render functions in other props, but I guess I'm in the minority.

very slowly. Separately, it must be possible to write a component that can work, simultaneously, on old React (with current context) and new React (with new context).

That's a good point about libraries needing to support both APIs at the same time

Implementation wise could we only leave the new implementation but polyfill the old API on top of it (with current broken semantics)?

Ooh maybe! Should at least consider it.

As long as I can trivially author a package that can work in both, I consider this addressed :-)

Someone will inevitably develop a component to do this composition automatically. I'm not worried about this drawback 👌

Agreed, and while I'm not super excited about the (below) suggested layering I'm certain that if one wasn't available I'd simply make one for use in apps to layer them together ala <ApplicationKitchenSinkConsumer> or something like that.

I agree with you about the tooling, it will happen because it is necessary.
Inspectors, stack trace and performance may be impacted by the layering though.

I honestly doubt this will happen all that often... 🤷‍♂️

Also consider that we can change the DevTools to display this however we like

@acdlite Indeed! DevTools improvements for HOC/layering makes sense beyond this RFC I think.

@kentcdodds I grep'ed the code of our apps this afternoon to review the impact of this RFC. We only have about 25 developers working with React, so consider my feedback with a grain of salt - we do not have the same stats as Facebook has :).
Our login button render method changes when logged or not authenticated (login or logout), on the theme (color), on being connected to the network or offline (disabled) and on the language of the user.

Overall, 20% of our context dependent render methods read 2 or more "atomic global state variable". If I understood correctly, the RFC suggests that if we want to only use React with no 3rd party tool, we should use 1 Context Consumer for each Context Providers we want to "consume from" - it sounds inconvenient to me.
But we already tooled ourselves with Redux and the RFC doesn't degrade our Redux experience so if it solves problems for others, go!

What about your React applications?

Most applications don't use the context API directly in every component that needs it and instead use an abstraction of some kind (HOC, render prop, or something else) to access context. In this case, the impact of the APIs verbosity is pretty minimal and the benefits of the new API make it easily worth it.

FWIW, @ryanflorence convinced me that it's easier to teach people a render prop accepts a function than it is to teach folks that the children prop can be a function. So in my teaching and open source I've been moving things over to render exclusively to hopefully make things more cohesive across the community while making it easier for people to pick up.

So, I'm in favor of using a prop called render personally.

Can't deny the learning curve. But to be fair, there's a learning curve (and arguably a steeper one) with the current context API. In addition, I think people will start to see render prop APIs before they have to start using context themselves. So it's unlikely that people will come to the context API before they've experienced a similar API with other libraries first.

Agreed with @kentcdodds, context has a learning curve. I think beginners coming across the proposed context API, making use of a render prop, will have a much easier time grokking what is happening, or where to start with it.

The bigger learning curve for the existing context API is trudging through myriad opinions about whether or not it's a good idea to use it, and, if so, how to use it safely.

This proposal being slightly more opinionated seems like it would result in a more gradual learning curve.

Could we make it required, but have the babel plugin add the displayName in the function call automatically? Best of both worlds?

const ThemeContext = React.createContext('light')

↓ ↓ ↓ babel plugin ↓ ↓ ↓

const ThemeContext = React.createContext('light', {
  displayName: 'ThemeContext',
})

Yeah, that's what I was trying to communicate. Maybe I'll just copy your snippet :D

I was thinking the displayName property would be assigned to the context object directly, as with other component types:

const ThemeContext = React.createContext('light')

↓ ↓ ↓ babel plugin ↓ ↓ ↓

const ThemeContext = React.createContext('light');
ThemeContext.displayName = 'ThemeContext';

Sounds fine. I thought making it part of the createContext call would make it easier to log an error. I don't really care how it's implemented though 😅

If you want to make the plugin to output static property, please make it in such fashion

const ThemeContext = React.createContext('light');
if (process.env.NODE_ENV === 'development') {
  ThemeContext.displayName = 'ThemeContext';
}

Otherwise statics will prevent tree-shaking. This could be configurable with wrap option for the transform which would default to true

Or, rather,

const ThemeContext = React.createContext('light');
if (process.env.NODE_ENV !== 'production') {
  ThemeContext.displayName = 'ThemeContext';
}

to match how we do other checks.

I noticed a few of our internal apps started using redux in a way such that they do not connect leaf components but instead just rely on the store being accessible on this.context and they grab things from there directly. I'm not saying this is a recommended approach just that it is one I've seen taken.

Given that behavior (or other direct context access and not using a context -> props hoist component) what would be the migration story for apps wanting to update and needing to replace that with the new <ThemeContext.Consumer>?

I realize that's a bit of a loaded question, but it seems like it wouldn't necessarily be something that could be code shifted at all, I imagine.

Intuitively, the migration to using connect()+props instead of reading straight out of context would be my first investigation.
A grep like:

$grep -hr this\.context src/

should give you an idea of the amount of effort required for this direction. Hope that helps!

Discussion around how long existing API would hang around, and implementation for that can be found #2 (comment)

If you give a static value (like store) to a context provider, consuming it in a child is basically free. So you can continue to use it for the this.context.store use case. The only difference is you'll have to use the render prop, instead of accessing it on the instance. Unfortunately this means there's no obvious way to codemod this, but it shouldn't be hard for a human to do the migration.

small typo: propagation

small typo: comparison

(if you plan to do a spellcheck at some point, no worries; I can stop pointing these out. I'm just reading through this document and pointing them out as I find them. By the way, this is great so far! Thank you! ✌️ )

Ha your spellchecks are appreciated :)