New commit phase lifecycle getSnapshotBeforeUpdate()

[bvaughn]

View formatted RFC

[quantizor]

How is this conceptually different from componentWillUpdate and componentWillMount? The similarity in the names makes me wonder if it makes sense to not deprecate the old methods and just reuse them in a semver major change.

[bvaughn]

How is this conceptually different from componentWillUpdate and componentWillMount?

componentWillUpdate and componentWillMount are called before render, and may be called for a render that is discarded because of a higher priority update. The newly proposed lifecycles would be called during the "commit" phase.

This draft docs page does a nice job in explaining the difference and why it matters to distinguish between the two: https://deploy-preview-587--reactjs.netlify.com/docs/strict-mode.html#detecting-unexpected-side-effects

The similarity in the names makes me wonder if it makes sense to not deprecate the old methods and just reuse them in a semver major change.

Reusing the old lifecycles would not be a good option for a couple of reasons. We currently plan to support the old lifecycle functionality in future releases, with an "UNSAFE_" prefix. Having similarly named methods with significantly different behavior would cause confusion and could make updating from 16 to 17 significantly more difficult.

We will also want these new lifecycles to be available in a minor release of 16- so people can begin upgrading to use async. For this reason as well the names must be unique.

Some familiarity is probably helpful though, since people already have a concept of mounting and updating.

[gaearon]

Specifically an important distinction is that people expect to be able to use componentWillMount for setting the initial state, but by the point these lifecycles run it's already too late. So changing what componentWillMount would be too confusing given all the existing code and examples that assume it happens before render (and on the server).

[gaearon]

I do wish we could recycle the existing names though since "will mount" and "will update" indeed seem like ideal names for these APIs 😢

[gaearon]

I guess we can add them under new names and then eventually find better names for them when/if we ever introduce a new lifecycle API.

[bvaughn]

I do wish we could recycle the existing names though since "will mount" and "will update" indeed seem like ideal names for these APIs 😢

Yeah ☹️I had the same thought. It's unfortunate that those names are tainted.

I'm definitely open for bike-shedding on better names than the ones I proposed initially though. 😄

[quantizor]

I like the idea of componentWillFlush, since that implies a stronger correlation with the "flush to DOM" part of the rendering flow.

Could we even perhaps just have a single event for both cases of mounting and updating? I'm curious if the extra API surface is necessary in practice.

[bvaughn]

I don't think a single lifecycle is a good idea, because prevProps and prevState would be null the first time it is called. I think the ergonomics of that might be a bit awkward.

[gaearon]

Maybe

componentWillFlushMount
componentWillFlushUpdate

[bvaughn]

I don't object too strongly to that, but I did intentionally avoid using "will" or "did" in the name because of the potential to cause confusion. With the (kind of awkward) caveat of componentWillUnmount, we're training people to be cautions of the "will" lifecycles, so I didn't feel great about adding new caveats.

[acdlite]

Internally, we use the prepare prefix to refer to this phase Nevermind, I misremembered: we use those for the "complete" phase

[trueadm]

componentDidRender?

[bvaughn]

componentDidRender was my first idea, actually 😁but I think it's important to have unique names for the did-mount case and the did-update case.

[bvaughn]

Renamed componentIsUpdating to getSnapshotBeforeUpdate and dropped the concept of the before-mount lifecycle. Doesn't seem useful for any case I can think of.

[TrySound]

Can getSnapshotBeforeUpdate be static? Refs can be kept in state.

[bvaughn]

Briefly considered making it static but decided against it because of refs. Felt odd to force refs to be stored in state.

[Swizec]

What about componentWillCommit? You mention a couple times that this happens during the commit phase before the component commits to the DOM.

[bvaughn]

I'm personally not a fan of the "componentWill" prefix for this use-case because (with the exception of componentWillUnmount) we're moving away from the other lifecycles with that name. Also, the current proposal is for this method to have a meaningful return value, which is something none of the other "componentWill" and "componentDid" methods do.

[flarnie]

One potential use case is in Draft.js, where we have two values we track that are outside of state and get updated roughly at the time this instance method would be called.

The current, short-term work-around we are doing is to update them in the componentDidMount and componentDidUpdate of a child component.

I think this could be a possible solution for the Draft.js use case, @sophiebits would be curious what you think when you are online again.

I also think that for Draft.js we also will not need the 'before-mount' call. Seems like Draft wasn't setting those flags in 'willMount'.

To be more specific about what Draft does by tracking those values - In one case it is intended to block stray 'select' events that fire in IE after Draft has already started a selection update, and in the other case it's caching the latest rendered state, so for both of those I don't think the initial mount is a problem.

[dantman]

Reparenting (RFC #28) could make use of this feature. <video>/<audio> elements stop playing in some browsers, inputs lose their focus, and scrollable elements lose their scroll position when you move them from one dom node to another dom node.

If these pieces of state are really important to a user, they could be snapshotted before reparenting using this RFC and restored after they are moved to a new dom node.

[sophiebits]

@flarnie For Draft I don't think we need any new lifecycles, instead maybe just we should refactor our componentDidMount calls so that we don't need to depend on the ordering as strongly. (That likely looks like the parent component controlling the selection handling instead of the leaves.)

[ivikash]

Should we also mention about shouldComponentUpdate and how will its return value true or false will impact this?

[ivikash]

envirnment -> environment

[ivikash]

lifecyle -> lifecycle

[ivikash]

favor -> favour

[bvaughn]

Both are valid spellings.

[bvaughn]

Should we also mention about shouldComponentUpdate and how will its return value true or false will impact this?

This lifecycle will be invoked if shouldComponentUpdate doesn't prevent a new render, just like componentDidUpdate. I don't know if this needs to be called out explicitly?

[bvaughn]

This RFC has entered the final comment period.

[bvaughn]

I've created a potential polyfill for this method with reactjs/react-lifecycles-compat/pull/1 but I don't think it's worth merging because of the reasons mentioned in the RFC.

[gaearon]

Can you expand on what

This would not work in all cases (e.g. Object.defineProperty syntax).

means?

[bvaughn]

Sure, sure. I meant cases like this:

class Test {
  constructor() {
    Object.defineProperty(this, "componentDidUpdate", {
      configurable: true,
      enumerable: true,
      writable: true,
      value: (prevProps, prevState) => {// ...
      }
    });
  }
}

Which is what Babel would generate for:

class Test {
  componentDidUpdate = (prevProps, prevState) => {
    // ...
  }
}

I could probably have worded this better.

[gaearon]

I think I'd be fine with not supporting that pattern. It shouldn't ever be necessary. As long as we document that in the polyfill README.

[bvaughn]

Gotcha.

It is at least possible to detect and error in this case (when cDU isn't on the prototype) which the polyfill POC PR does.

[bvaughn]

Thanks for the suggestions and discussion, everyone.