As the RFC stands it seems written towards solving problems for one, very specific, workflow.

Maybe that’s just the examples.

The resulting interface though seems extremely verbose & complicated comparatively to the one it replaces.

I like the goal of this though. The idea is good.

Thanks for the input!

As the RFC stands it seems written towards solving problems for one, very specific, workflow.

The goal is to solve all workflows/use-cases in a way that's safer by default (or maybe, in a way that has fewer potential pitfalls, or requires less advanced knowledge to use correctly). If you know of use-cases that it couldn't handle, this is a great time to discuss them!

The resulting interface though seems extremely verbose & complicated comparatively to the one it replaces.

Does the proposed API actually increase verbosity or complexity? I think it just moves things around, but maybe I'm stuck looking at it with a stale perspective.

I feel this proposal has great benefits over what we have now, but ultimately should probably be merged into an entirely new component API that doesn't involve classes at all. Furthermore, to unify with our other efforts, i.e. compilation of React components, by adding more to already complicated class component will makes things even more complicated to handle.

In my opinion we need a new component API that offers:

• the same flexibility as before, but with some sensible constraints
• an async-first approach to dealing with lifecycle events like outlined in this proposal
• a compiler friendly approach to dealing with things like component folding, bytecode interpretation and dead-code elimination

Thanks Dom.

I feel this proposal has great benefits over what we have now, but ultimately should probably be merged into an entirely new component API that doesn't involve classes at all.

I'm not sure what the right answer is, but the advantage of this proposal over what you're describing is that it provides anincremental path for existing components to take advantage of async. A completely new, non-class-based API seems both harder to designand much larger effort to adopt.

That being said, I recognize that I can't appreciate the compiler difficulties presented by the class component API like you can.

The goal of this proposal is not to be the final "React.next" API. It's to provide a reasonable upgrade path for a large set of components e.g. in a codemoddable way.

The assumption is that:

1. Whatever the next API change is will be significantly different enough that you can't just codemod your way there. E.g. leaking an instance alone would bail.
2. It is a relative easy manual change to move to these new life-cycles.
3. There are significantly fewer components that use these particular life-cycles than there are total components.

It is also much easier to adopt full async in a large codebase this way and a small ever growing portion being compiler compatible with the new API.

Now whether it is worth doing both steps is arguable but that's the idea.

Let me take a step back and state what I feel the ultimate goal would be for me: in the future, we don't have class components anymore and state/lifecycles should instead be handled by another mechanism that doesn't force us to break out a declarative render phase into an imperative OO class phase.

That sounds bonkers, but will likely be a future RFC from me once I have time to properly explain it. That being said, incrementally, I feel we shouldstart adding constraints to components:

• Seal the class instance after construction, so instance variables are no longer possible
• Come up with a new lifecycle interface (like outlined in this PR) that replaces the current lifecycle methods

@trueadm : If instance variables are no longer possible, and lifecycle methods are going away, how would React components handle non-React interop? The standard example would be:

classMyPluginWrapperextendsComponent{componentDidMount(){this.$theDiv=$(this.theDiv)this.$theDiv.somePlugin("create");}componentDidUpdate(){this.$theDiv.somePlugin("update",this.props.someValue);}componentWillUnmount(){this.$theDiv.somePlugin("destroy");}render(){return<divref={theDiv=>this.theDiv=theDiv}/>;}}

In this use case, references to non-React values are stored as instance fields, and created/updated/destroyed in lifecycle methods.

Thanks for your work on this RFC!

I may just not have enough background in the async discussion (is there a place where those ideas are laid out?), but I have to say that I found this RFC very difficult to understand. A few suggestions that probably would have made it easier for me and maybe might make it easier for others:

1. Move the motivation/description of problem to the top. The code sample is long, and was somewhat hard to grok when I had no idea what was being changed or why.
2. I'd suggest beefing up the description of the three problems you listed in Common Problems. It sounds like some of these are programmer errors; why aren't they currently causing problems? Or are they?
3. Consider separating the code sample out into multiple examples, ideally each connected to a Common Problem. For me, it was very difficult to keep all the things going on in those lifecycle method in my head at once.
4. It seems like there are going to be changes to the order of lifecycle methods in an async version, and this RFC assumes the reader understands what those are. (I might be wrong here; it just seemed implied by some of the text.) If so, explaining that or linking to a clear explanation would be great!

Again, thanks for thinking through these issues and for all your work. Also, I apologize for the stuff I just have totally misunderstood. I'm excited to see async moving forward!

Note: this isn't really that relevant to this PR proposal.

@markerikson I'm glad you asked :) lately I've been using a lot of ReasonReact and the place for refs is most definitely state, which would fit nicely into mycreateRef proposal:facebook/react#11555.

For more information on how ReasonReact deals with refs:https://reasonml.github.io/reason-react/docs/en/react-ref.html

Move the motivation/description of problem to the top.

That's useful feedback for the RFC process in general. This proposal just follows thestandard template.

It seems like there are going to be changes to the order of lifecycle methods in an async version, and this RFC assumes the reader understands what those are.

I don't think I meant to imply any change in ordering. Could you clarify where you thought this?

Actually@aickin, do you mind splitting up your comments and adding them inline? Large, multi-bullet comments are hard to respond to in a single thread like this. 😄

I don't think I meant to imply any change in ordering. Could you clarify where you thought this?

I think it was a vague impression I got from the following ideas:

1. The async experiment you explained showed non-idempotent function calls in functions likecomponentWillMount, and you mentioned it's a problem that they could get called "multiple times".
2. Those types of non-idempotent function calls should move to the new static functions, which will solve the problem.

That implied to me that the new static functions are called in a different way than their previous instance method analogues, which made me think that there are more changes to lifecycle order/number of calls. Honestly, though, it was a very rough impression that I may have just totally misread.

@aickin, do you mind splitting up your comments and adding them inline? Large, multi-bullet comments are hard to respond to in a single thread like this.

Sure, sorry!

ETA: I moved my big list of nits out from this discussion into line comments in the document. I hope they are genuinely helpful, and please let me know if they are not!

@trueadm : so just to clarify, the place to storeanything a component needs to reference would now be in "state", even if those values aren't directly related to re-rendering? The rule of thumb so far has been "if it relates to rendering, put it inthis.state, otherwise store it asthis.someValue".

Note: this isn't really that relevant to this PR proposal.

@markerikson In my opinion, something in state doesn't have to mean it anything to do with rendering. I think we have an opportunity to change that in the future and simplify things too. I attempted to tackle these problems with Inferno and they were very received – so I feel we can do better here.

@trueadm : I think the reason for that rule of thumb comes from this:

• You shouldn't mutatethis.state
• CallingsetState() queues a state update, but also a re-render
• Updating a jQuery plugin or some other non-HTML-ish API isn't something that goes inrender, so you need to apply the updates to that in another lifecycle method
• Actual re-rendering is almost definitely wasted CPU cycles for this scenario
• Thus, you don't want to callsetState() at all, so that you don't trigger pointless re-renders

If you've got some ideas that work well as an alternative to that mindset, I'm certainly curious to see what you have in mind.

WillgetDerivedStateFromNextProps be called for the initial render? The RFC only mentions it as a replacement forcWRP.

It will not be called in the current implementation but it is under consideration.

The new lifecycle allows to use with functional components, as its static and js allows it. It must be documented what React will do in this case.

Lifecycle hooks apply to class components only. I thinkthe existing documentation already makes this pretty clear. Once we add docs for the new (static) lifecycle methods we'll hopefully make that clear as well.

The docs are a community effort too, so if you think it's not clear enough- PRs are welcome.

Edit: I've added a TODO to my PR to detect and warn about this case in DEV mode.

WillgetDerivedStateFromNextProps be called for the initial render? The RFC only mentions it as a replacement forcWRP.

The RFC isslightly out of date with respect to a couple of things. (I'll probably update it once I've posted an implementation PR.) This is one of them.

Our current thinking is thatgetDerivedStateFromPropswill be called after construction for the initial state.

Really glad to see this being discussed because it's a point we've debated/worked on a great deal 👍

This is a fairly lengthy thread so apologies if my comments end up repeating any of the points I've missed here or in the various code reviews.

PureComponent's

The one point I haven't seen discussed here is that ofPureComponent's. We usePureComponent's across the board (because we use ImmutableJS exclusively) and one of the issues we've found with the current API is the sequence of events when it comes to updating props. The sequence of events from the perspective of a child component is as follows:

componentWillReceivePropsshouldComponentUpdatecomponentWillUpdaterendercomponentDidUpdate

As things currently stand in React, even if props donot change (with respect to the definition ofPureComponent) then the child'scWRP still fires. We handle this by actually proxying calls (we do a load of plumbing/overriding in a common base class) and only forwarding thecWRP call to our components if we determine the props have changed (calculated by the same shallow comparison asPureComponent'sshouldComponentUpdate, because only in this situation would the subsequentshouldComponentUpdate return true). Whilst this does introduce some inefficiency, it does mean thatcWRP has much clearer semantics in our components.

Common props handling

As things currently stand, we almost always delegate from a component'scWM andcWRP to acommonProps() method which has access to two helper properties:props andprevProps.props gives us the new/next props,prevProps (as its name suggests) gives us the previous props.prevProps can only be called from within (transitively)cWM orcWRP (although I think this restriction is superfluous).

(We implemented these as properties given the pre-existingprops property incidentally.)

commonProps() is the point where we derive state from props (per this proposal), hence having access to the previous props is critical at this point (even though we know that the propshave changed, because of the use ofPureComponent).

MakingsetState sync

We have also madesetState() a synchronous operation. I asked a question about why it was decided it should be async inthe React Discussion Forums; there is also anopen issue discussing this point.

We found thatsetState()not being synchronous leads to either mistakes or rather convoluted chained code where a value fornextState is passed from method to method, with one of those methods finally callingsetState().

Memoized values

I've seen the discussion about putting memoizing logic intorender(). This is fine except in those situations where you need the memoized values in places other thanrender() (i.e. callbacks etc). In which case these memoized values have to go into state. But this is fine: because your trigger for recalculating the memoized values is either a change in props (cWM/cWRP) or a change in state. In either case the component is going to re-render (by definition), so updating state further is effectively at no extra cost. Again this is where havingsetState() be synchronous helps to keep code simpler in our experience.

How this maps to the current proposal

All of that said, I have the following questions with respect to this proposal/thread:

• getDerivedStateFromNextProps sounds very similar to the approach we follow withcommonProps. Just to confirm, this method will only fire when the props have actually changed (see point aboutPureComponent's)?
• I understand the point aboutcomponentDidMount being a better place to setup event listeners. As mentioned above,cWM is one of the entry points tocommonProps (our method for deriving state from props). So are you proposing that the first timegetDerivedStateFromNextProps would be called is after the initialrender?
• Any thoughts on the question aboutsetState? Or indeed our approach of having made it synchronous? We can continue that discussion in the linked issue if you feel it's a better place for it, but as you can see we found it very much linked to the lifecycle points in this thread

Thanks in advance.

getDerivedStateFromNextProps sounds very similar to the approach we follow withcommonProps. Just to confirm, this method will only fire when the props have actually changed (see point aboutPureComponent's)?

This method will be called at the same time and in the same cases ascomponentWillReceiveProps is currently. This RFC does not propose any changes to the method with regard to shallow props comparison.

I understand the point aboutcomponentDidMount being a better place to setup event listeners. As mentioned above,cWM is one of the entry points tocommonProps (our method for deriving state from props). So are you proposing that the first timegetDerivedStateFromNextProps would be called is after the initial render?

As mentioned inthe comment above yours, our current thinking is thatgetDerivedStateFromProps will be called after construction (for the initial state) as well.

Any thoughts on the question aboutsetState? Or indeed our approach of having made it synchronous? We can continue that discussion in the linked issue if you feel it's a better place for it, but as you can see we found it very much linked to the lifecycle points in this thread

I understand why you mention it, but I don't think this (already pretty noisy) discussion is the best place to have this chat. 😄

Thanks for the quick response@bvaughn

This method [getDerivedStateFromProps] will be called at the same time and in the same cases as componentWillReceiveProps is currently. This RFC does not propose any changes to the method with regard to shallow props comparison.

Is there scope for extending this proposal to include consideration ofPureComponent? Because otherwise logic withingetDerivedStateFromProps will be called needlessly, negating the point/benefit ofPureComponent.We can of course continue to proxy as I described, but I wonder whether we kill two birds with one stone and fixPureComponent as part of this proposal.

As mentioned in the comment above yours, our current thinking is thatgetDerivedStateFromProps will be called after construction (for the initial state) as well.

Sorry, I've just now seen that response. Makes total sense. Sounds likegetDerivedStateFromProps will be a slot in replacement for ourcommonProps, notwithstanding the point aboutPureComponent above.

I understand why you mention it, but I don't think this (already pretty noisy) discussion is the best place to have this chat.

Noted 👍 - perhaps I can encourage you /@gaearon / others to chime in on either the linked issue or the React Discuss thread?

Is there scope for extending this proposal to include consideration ofPureComponent? Because otherwise logic withingetDerivedStateFromProps will be called needlessly, negating the point/benefit ofPureComponent.

It seems like you're concerned about the price of computing updatedstate fromprops ifprops haven't changed? A common pattern for dealing with this is to only update values instate if the correspondingprops values have changed. (Assuming your component has more than one prop, you'd likely need to do this checkanyway to ensure that theprops yourstate care about have changed.)

I don't think this negates the value ofPureComponent at all. Typically, the most expensive part of a component updating is therender (and all of the subsequent children updates).

As for the other discussion thread, I'm sure one of us will try to comment when we can. There's a lot of discussion threads and not many of us though. 😅 Thanks for bring it to attention again. Sorry!

I really approve of this proposal!

When it comes tocomponentWillMount and SSR specifically I have personally never seen a valid use-case for cWM in a SSR-app, but I have seen tons of invalid uses of it due to misconceptions. Marking it unsafe would definitely help clear this up.

@bvaughn

It seems like you're concerned about the price of computing updated state from props if props haven't changed? A common pattern for dealing with this is to only update values in state if the corresponding props values have changed. (Assuming your component has more than one prop, you'd likely need to do this check anyway to ensure that the props your state care about have changed.)

I don't think this negates the value of PureComponent at all. Typically, the most expensive part of a component updating is the render (and all of the subsequent children updates).

You make a very fair point; we do indeed end up comparing fields betweenprops andprevProps to conditionally recalculate derived state.

So I think my "unease" simplifies then to the incongruence ofgetDerivedStateFromProps being called when, as aPureComponent, youknow the props haven't changed (becauseshallowEqual(nextProps, prevProps) === true). Otherwise don't we end up having to document that it's called regardless?

But then perhaps I'm only pushing this because I was surprised when I first stumbled across this in the current implementation!

As for the other discussion thread, I'm sure one of us will try to comment when we can. There's a lot of discussion threads and not many of us though. 😅 Thanks for bring it to attention again. Sorry!

No apology needed :)

Otherwise don't we end up having to document that it's called regardless?

But then perhaps I'm only pushing this because I was surprised when I first stumbled across this in the current implementation!

For what it's worth,the docs do point this out 😄

componentWillReceiveProps()

Note that React may call this method even if the props have not changed, so make sure to compare the current and next values if you only want to handle changes. This may occur when the parent component causes your component to re-render.

For what it's worth, the docs do point this out 😄

🤦‍♂️ - I think I'll go and take a break :)

Thanks, I must have missed this before.

No worries. It happens~

This RFC has been implemented viafacebook/react/pull/12028 which was merged this morning.

When we can expect 16.3 release?

To be determined.