Render arguments

[micnic]

A proposal to add arguments for the .render() method

Todo:

• Add implementation section
• Decide if the context argument should be part of this proposal
• Decide between render(props, state) and render({ props, state })

[TrySound]

Ref facebook/react#1387

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[gaearon]

Sorry if I sound patronizing but this doesn't answer the question. The question is: how do we reflect that there are two (almost but not quite) equivalent ways of doing things in the documentation, which approach we'll teach first, how do explain why the lifecycle methods or event handlers don't receive current props/state (or should they?), etc.

[micnic]

I agree, misunderstood the question, will add a clarification that it should be studied along with this. approach and explained the edge cases

[gaearon]

Have you considered this in depth? They're not strictly equivalent because of how JS objects work: facebook/react#1387 (comment).

Are you sure this isn't an issue in practice? Can you demonstrate why?

[micnic]

I would consider this as an edge case, not really a drawback, but for the sake of completeness will add that this approach can involve potential hazard if the cached props or state are being used in event handlers or any other async operations, and as a best practice to avoid this case would be to not create any closure inside the render method.

[kyleshevlin]

It's worth noting that Preact uses this API in their render function. It's convenient, and if you have relatively few keys on props and state to destructure, it's nice to be able to do so in the method args. However, after only a few keys, it can get quite long (Granted, equally long inside the render method if you destructure there). Since this is primarily a DX change, I thought it should be noted.

[BTMPL]

With the advent of "do not focus on micro optimisation, write inline functions in callbacks" mindset by some prominent developers I think this would be quite a big issue and would lead to a lot of errors done by newcommers.

IMO it would introduce an unecessary learning curve.

[micnic]

This "learning curve" is already introduced in the react docs in event handling:

... In most cases, this is fine. However, if this callback is passed as a prop to lower components, those components might do an extra re-rendering. We generally recommend binding in the constructor or using the class fields syntax, to avoid this sort of performance problem.

This recommendation can be reformulated in the same way, but I think it must be added to cover this edge case.

[ghost]

I'd also like to see this approach on all lifecycle hooks in React component, such as: componentWillReceiveProps(currentProps, prevProps)

This proposal will be more close to purity of react functional nature, but it will be a breaking change I guess

[jamesplease]

Imo this feature isn’t useful enough to justify a breaking change to the signature of every lifecycle method, so I’m not as keen on seeing it extend beyond render.

[kyleshevlin]

I agree with @jmeas that we shouldn't update the other lifecycle methods. It would be a breaking change because you'd be affecting argument arity and order. I'm not convinced of how necessary this RFC is, and I would be hesitant for it to have a greater impact on the other methods.

Having used both React and Preact quite a bit, I kind of prefer not having the convenience of props, state, context in the render method, if only because you have to use this in all the other methods, save for the constructor(props) scenario.

Sometimes consistency is really the best developer experience.

[JNaftali]

Aside from convenience/dx are there any downsides to code that looks like this?

function doRender(props, state) {
  return <div />
}

class MyComponent extends React.Component {
  constructor(props) {
    super(props)
    this.doRender = doRender.bind(this)
  }

  render() {
    this.doRender(this.props, this.state)
  }
}

I'm not arguing against this proposal - I really like this feature in Preact. But what would the differences be between this proposal and a Babel transform that did something like the above?

EDIT because I forgot about this

[micnic]

@JNaftali your example is missing the return statement in the render() method. If you compare the approach from the proposal and this example, first of all it's the runtime performance impact, you have to bind a method to this on component instance creation and this "proxy" function has to be called everytime a render happens, I guess the JS engines are adding some inline optimizations, but it's still less performant that proposal's approach.

The proposal's purpose is to add an alternative approach to use data stored in the props and state in a cleaner way, nothing more.

[alexrussell]

I quite like this proposal because it brings the function-based component methodology and the class-based one together - you can turn a function component into a class one by just adding the class wrapping around the render function, and then add whatever you need regarding state or lifecycle hooks.

[jamiebuilds]

Alternative API:

render({ props, state, context }) {
  // ...
}

If you just wanted state or context you don't have to include the other arguments:

render(props, state, context) {...}
// vs
render({ context }) {...}

[j-f1]

What about having React just call render with this as the first argument?

[jamiebuilds]

That's sorta what @mjackson's my-react does: https://github.com/mjackson/my-react

[micnic]

I like this approach, will add it as an alternative for this proposal

[sonhanguyen]

personally, I prefer keeping the order props, context like it currently is in functional components and constructors. I don't mind throwing in state as the 3rd but to me, it's less relevant. props and context are parts of the contract between components, state is just an internal thing.

[j-f1]

IMO it should stay props, state, context because the context docs begin with a large section explaining that you shouldn’t use it, and it would be confusing and annoying to have to write render({ someProp }, _, { someState }) { /* ... */ } every time you need access to state.

[quantizor]

I don't mind throwing in state as the 3rd but to me, it's less relevant.

Context is less valuable than state IMO since the 99% case would not have people drawing anything off context unless you're a library author shipping a Provider/HOC combo. Local state however is frequently utilized to inform rendering.

That being said, it's cheap to supply context as a third argument... so not worth wrestling over.

[TrySound]

@probablyup @j-f1 @sonhanguyen Guys, new context api is merged. The old one will be removed in v17. Just do not bikeshed about it here.

[streamich]

Please also add HyperScript function h in there.

render (h, props, state, context) {

}

, where

const h = React.createElement;

[TrySound]

@streamich Oh, and also createPortal would be nice.

render (h, cp, props, state, context) { }

and even createFactory (who cares?)

render (h, cf, cp, props, state, context) { }

Also it's not "HyperScript" function. Hyperscript is wrapper with own signature.

[kyleshevlin]

I don't think either of the last two comments were very constructive. Let's not turn this thread into making fun of one another. Let's focus on figuring out whether this RFC is worth pursuing and how. Here are the things that will drive this forward:

1. Is there consensus that there is value in adding arguments to the render() method?

2. If so, what should those arguments be, and in what order?

Focus on that. Let's keep it simple and civil here.

[jamesplease]

Is there consensus that there is value in adding arguments to the render() method?

I initially didn't feel too strongly about this (and was leaning slightly in the direction of "yes"), but after reading your comment here, I was persuaded and would rather not see these changes to the render method.

It is interesting because this change increases consistency when writing both function components and class components, but decreases consistency when writing a single class component.

I'd rather keep the class component consistent with itself.

[streamich]

@kyleshevlin I don't know why you think my comment was not constructive.

I don't think either of the last two comments were very constructive.

Having hyperscript function h being dependency injected is very useful, you also may want to do that for stateless function components. That way you can create self contained templates, that can be used by any library, rather then JSX being compiled to React.createElement hidden dependency.

With regards to argument order: (1) obviously props has to be the first argument to make it consistent with stateless components; or (2) you could pass in an object which contains everything.

render ({h, props, state}) {
  // ...
}

In which case you don't care about the order and it can be extended in the future.

[kyleshevlin]

I personally think dependency injection of an h function is not in the spirit of the RFC, and unsure how that fits in the ecosystem at large. I think most users are happy with the de facto JSX/React.createElement transpilation. Your use case seems to me quite rare.

As for an object, it does get rid of any issue with ordering, but is inconsistent with SFCs and Preact's API.

[streamich]

I think most users are happy with...

Most users are happy with the de facto state of anything for that matter, as React is the most popular lib, but it does not mean there is no room for improvement.

As for an object, it does get rid of any issue with ordering, but is inconsistent with SFCs...

Well, it depends how you look at it. You could say that the entire object is basically props. In that case it makes the render method basically a stateless component, you could even do:

SomeComponent.prototype.render = MyStatelessComponent;

Maybe state could be even inserted into the props:

render ({state, ...props}, context) {
  // ...
}

That's your stateless component right there!

... and Preact's API.

Why do we care about Preact here? There is preact-compat that will take care of this.

Your use case seems to me quite rare.

I don't know if it is rare, but think about this: stateless components are supposed to be "pure" functions that receive props and return markup. But they are not, they have a hidden dependency React.createElement. So to create really pure stateless components you need to either inject h, or transpile JSX to something that has no dependencies, say JSON-ML:

<div>Hello world</div>

to

['div', null, 'Hello world']

[kyleshevlin]

I think there might be some merit to your suggestions @streamich, but personally I believe its use case is so small, I just don't know how you'd drive consensus for it. But my opinion on the matter is just that, opinion. So if you'd like to champion that idea, I think you should. It might gain more traction in its own RFC since it's a dependency injection of something basically unseen in React, and arguably already exists if you choose to use the createElement method.

As for my opinion on the merits of this RFC in general, I do not believe adding arguments to render is a worthwhile pursuit. I think it adds unnecessary complexity. For example, imagine you are just starting with React, you add some view logic in the render() method that requires this.props or this.state. By not having these injected into the method, you can very easily refactor bits of your render logic into their own method without needing to add this or make other adjustments.

One of the joys of React is that the API is so tiny. I personally would hate to see it gain bloat from things that are just tiny comforts that don't add any real substance to the library. I could easily just be unwise regarding the merits of this RFC, but I just don't see a worthwhile gain made by pursuing it.

[micnic]

As it was pointed out, the new context API has been merged and is not compatible with what is presented here, I will remove the context argument from this proposal.

[streamich]

What about

render({state, ...props}) {

[TrySound]

state is not props and can't be part of it.

[streamich]

Of course, it can, Redux and MobX merge state into props all the time.

[j-f1]

What would happen if I used a state prop?

[TrySound]

@streamich Rephrase: local state is not props and can't be part of it. Redux state is external state which is passed from parent component.

[streamich]

@vcarl yes, you are right; that would just add one more mine to the key, ref, store, etc. minefield

[j-f1]

Why store?

[streamich]

Because of Redux.

[j-f1]

But if I don’t use Redux, I can pass store to my components. If store is “part of props,” then you can never use the state prop.

[streamich]

@j-f1 that is pretty obvious

[adrianhelvik]

The greatest benefit of this proposal imho is to easily convert stateless components to class components, so I second only providing the props argument. State is class only, so including it only adds conflicting APIs. And it could easily be added later.

[jchitel]

Right now the entire component lifecycle (barring getDerivedStateFromProps, which is static) has a level of consistency; whatever data is currently inside a component is accessible via this, and any data that is either no longer accessible or not yet accessible via this is passed to each method as a parameter:

API	Data In (parameters)	Current Data (this)	Result (mutation)/Data Out (return value)
constructor(props)	Initial props	Nothing	Component has initialized this.props and this.state
componentDidMount()	Nothing	Current props and state	Component has fully initialized this.state (and things like refs, subscriptions, etc.)
shouldComponentUpdate(nextProps, nextState)	Next props and state	Current props and state	Return boolean for whether to re-render
getSnapshotBeforeUpdate(prevProps, prevState)	Previous props and state	Current props and state	Return snapshot for componentDidUpdate()
componentDidUpdate(prevProps, prevState, snapshot)	Previous props and state, snapshot	Current props and state	Call external APIs in response to an update
componentWillUnmount()	Nothing	Current props and state	Clean up resources or subscriptions
componentDidCatch(err, info)	Error data	Current props and state	Respond to errors by updating state, calling external APIs
render()	Nothing	Current props and state	Return node to render

You'll notice that the one thing in common with all of the methods (with the obvious exception of the constructor) is that the current props and state are accessible on this.props and this.state. This will always be true for every instance method. It simply doesn't make sense to make the current props and state accessible some other way when they will always be accessible in one place.

If all that is desired is not having to add this. to everything, then one line can easily take care of that:

const { props, state } = this;

Likewise with destructuring the props and state themselves:

const { prop1, prop2 } = this.props;
const { state1, state2 } = this.state;

It would begin introducing inconsistencies with the existing API, as well as adding unneeded extra complexity; having two ways to do something will make new users question the "right way" to do something.

[micnic]

Based on the feedback received here I decided to discontinue this RFC. Thanks everyone for constructive comments and sharing your opinions on this topic. I have to agree that this proposal would introduce a degree of inconsistency in the react API and there is no real gain in using this approach. As it was pointed out destructuring is enough for working with props and state.

[nickmccurdy]

This is not inconsistent. Sometimes multiple APIs need to be added for the same approach to have more consistent behavior between different use cases. Additionally, React already is intentionally consistent, for example: class vs function components, three ref APIs, createElement or JSX.

Personally I see no reason against this. If someone doesn't want to use the args because they seem inconsistent, they don't have to use them, they can keep using this, and we don't have to break back compatability. But keeping these outside args makes it unnecessarily difficult for beginners to use class components without any clear advantages as far as I can tell. I constantly see people trying to use this.props in function components and the props parameter in class components, and I still see no technical advantages to keeping them separate.

Please consider re-opening.

[gaearon]

If someone doesn't want to use the args because they seem inconsistent, they don't have to use them

I think the bigger problem is just that props and this.props can be different inside a closure. It's something you can live with if you're doing destructuring early yourself. But if React gives you both props and this.props, and they can potentially get out of sync, I think it's a sign of a flawed API.

We have other ideas on how to solve this. We'll be sharing more in the coming months.

[nickmccurdy]

Thanks, that's a good point. There's still the inconsistency of the behavior of argument binding in function component closure handlers. However, we have some other good alternative solutions like what ReasonReact is doing by merging class-like features into function components.