Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

An update on async rendering #596

Merged
merged 64 commits into from
Mar 27, 2018
Merged

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Feb 6, 2018

Work in progress...

Formatted "update on async rendering"

TODO before merging

@reactjs-bot
Copy link

reactjs-bot commented Feb 6, 2018

Deploy preview for reactjs ready!

Built with commit b824bd2

https://deploy-preview-596--reactjs.netlify.com

@bvaughn bvaughn changed the title [WIP] An update on async rendering An update on async rendering Feb 6, 2018
Copy link
Member

@wbinnssmith wbinnssmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usecase-oriented sections are so good! Maybe they could be a part of a "patterns" section -- or similar like the recipes you mention -- in the docs in the future. There's already a lot of good content in the quick start that could belong there too.

* **16.4**: Enable deprecation warning for `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate`. (Both the old lifecycle names and the new aliases will work in this release.)
* **17.0**: Remove `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate` . (Only the new "UNSAFE_" lifecycle names will work in this release.)

In this post, we will explore some of the potential capabilities of async rendering, and we'll outline a migration plan for components that rely on the deprecated lifecycles.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should mention the codemod you added in reactjs/react-codemod#195! 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was uncertain about this, but leaning toward mentioning the codemod along with the release of 16.4. My reasoning is that 16.4 will be when the deprecation warnings are enabled, and the codemod is really only a bandaid fix for those. This release (16.3) is more focused on maintainers who want to fix their projects in the "right" way going forward (which won't be using the codemod, but rather following the recipes). So I thought mentioning it might lead people down the wrong path potentially?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah I can totally see that. makes sense to me!


We found that asynchronous rendering can help in several ways. For example:

1. As users navigate within an app, newly displayed components often have asynchronous dependencies (including data, images, and code splitting). This can lead to a "cascade of spinners" as the data loads. We'd like to make it easier for product developers to express asynchronous dependencies of components- keeping the old UI "alive" for a certain period while the new UI is not "ready" yet. React could render this new UI in the background and provide a declarative way to show a spinner if it takes more than a second.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

provide a declarative way to show a spinner if it takes more than a second.

maybe s/spinner/loading indicator?


1. As users navigate within an app, newly displayed components often have asynchronous dependencies (including data, images, and code splitting). This can lead to a "cascade of spinners" as the data loads. We'd like to make it easier for product developers to express asynchronous dependencies of components- keeping the old UI "alive" for a certain period while the new UI is not "ready" yet. React could render this new UI in the background and provide a declarative way to show a spinner if it takes more than a second.
2. Fast updates within a short timeframe often cause jank because React processes each update individually. We'd like to automatically "combine" updates within a few hundred milliseconds when possible so that there is less re-rendering.
3. Some updates are inherently "less important" than others. For example, if you're writing a live-updating search filter input like [this](https://zeit.co/blog/domains-search-web#asynchronous-rendering), it is essential that the input is updated immediately (within a few milliseconds). Re-rendering the result list can be done later, and should not block the thread or cause stutter when typing. It would be nice if React had a way to mark the latter updates as having a "lower priority".
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some updates are inherently "less important" than others.

imo the quotation marks aren't really necessary, though they could be around important, as in less "important"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same re: "lower priority"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe also worth adding

Note that even debouncing the input doesn’t help because if the rendering is synchronous, like in React today, a keystroke can’t interrupt the list rendering if it already started. Asynchronous rendering solves this by splitting rendering into small chunks that can be paused and later restarted.

Here is an example of a component that subscribes to an external event dispatcher when mounting:
`embed:update-on-async-rendering/adding-event-listeners-before.js`

Unfortunately, this can cause memory leaks for server rendering (where `componentWillUnmount` will never be called) and async rendering (where rendering might be interrupted before it completes, causing `componentWillUnmount` not to be called).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this and the lifecycle method is being deprecated 😝

might be worth calling out that there's more symmetry between didMount and willUnmount both taking place while the component is mounted and refs and dom apis are available.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like this way of framing it. I'm a little unsure of the wording, but I'll take a stab at it.


We found that asynchronous rendering can help in several ways. For example:

1. As users navigate within an app, newly displayed components often have asynchronous dependencies (including data, images, and code splitting). This can lead to a "cascade of spinners" as the data loads. We'd like to make it easier for product developers to express asynchronous dependencies of components- keeping the old UI "alive" for a certain period while the new UI is not "ready" yet. React could render this new UI in the background and provide a declarative way to show a spinner if it takes more than a second.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can expand a bit. To clarify it’s not only perf oriented, and also why exactly the perf suffers

This leads to a lot of boilerplate code managing data fetching and displaying the loading states. It can also lead to a “cascade of spinners” as the data loads, causing DOM reflows and janky user experience.


## Updating class components

#### If you're an application developer, **you don't have to do anything about the deprecated methods yet**. The primary purpose of this update (v16.3) is to enable OSS maintainers to update their libraries in advance of any deprecation warnings. Those warnings will be enabled with the next minor release, v16.4.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OSS is not universally understood


In this post, we will explore some of the potential capabilities of async rendering, and we'll outline a migration plan for components that rely on the deprecated lifecycles.

## What can Asynchronous Rendering do?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about removing capitalization here? I originally capitalized all words but if we don't do this for headers in general, let's just keep "asynchronous rendering" lowercase as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It's kind of arbitrary.

* `componentWillReceiveProps`
* `componentWillUpdate`

Because of this, we have decided to rename these lifecycles—(adding an "UNSAFE_" prefix)—in a future release. The plan for this is as follows:
Copy link
Member

@gaearon gaearon Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of "The plan ...", "React follows semantic versioning, so the migration path is gradual:"

* **17.0**: Remove `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate` . (Only the new "UNSAFE_" lifecycle names will work in this release.)

In this post, we will explore some of the potential capabilities of async rendering, and we'll outline a migration plan for components that rely on the deprecated lifecycles.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe "on the deprecated lifecycles" -> "on these legacy lifecycles". Since they're not technically deprecated yet. Also too many people read "deprecated" as "immediately removed" so I prefer other words when possible.


* **16.3**: Introduce aliases for the unsafe lifecycles, `UNSAFE_componentWillMount`, `UNSAFE_componentWillReceiveProps`, and `UNSAFE_componentWillUpdate`. (Both the old lifecycle names and the new aliases will work in this release.)
* **16.4**: Enable deprecation warning for `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate`. (Both the old lifecycle names and the new aliases will work in this release.)
* **17.0**: Remove `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate` . (Only the new "UNSAFE_" lifecycle names will work in this release.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any vague sense of timing we could give here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm reluctant to make any predictions on timing.

I could say "within a week or two", "within a month or two", etc. if that would be helpful?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eh, I guess not. I think the most valuable would be to say 17 is at least X months away but honestly I have no idea

* `componentWillReceiveProps`
* `componentWillUpdate`

Because of this, we have decided to rename these lifecycles—(adding an "UNSAFE_" prefix)—in a future release. The plan for this is as follows:
Copy link
Member

@gaearon gaearon Feb 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a native speaker but I find using both dashes and parens together a bit hard to scan. I think that, when possible, we should stick to simple sentence structures that are easily accessible to non-native speakers (note I break this rule way too often myself).

Here is one potential rewording:

Because of this, we are adding an “UNSAFE_” prefix to these lifecycles in a future release. [...]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is helpful feedback to me. Thanks!

Here is an example of a component that subscribes to an external event dispatcher when mounting:
`embed:update-on-async-rendering/adding-event-listeners-before.js`

Unfortunately, this can cause memory leaks for server rendering (where `componentWillUnmount` will never be called) and async rendering (where rendering might be interrupted before it completes, causing `componentWillUnmount` not to be called).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice hammering of the point that most of these patterns are already SSR-unsafe 👍

derivedData: computeDerivedState(
nextProps
),
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My overall feedback on these examples: I see what you are trying to do but I think their vagueness is more distracting than helping. I would vastly prefer a specific example (that also shows when I was supposed to use this pattern in the first place).

For example, before:

  state = {
    isScrollingDown: false,
  };

  componentWillReceiveProps(nextProps) {
    if (this.props.currentRow === nextProps.currentRow) {
      return;
    }
    this.setState({
      isScrollingDown: nextProps.currentRow > this.props.currentRow
    });
  }

After:

  state = {};

  static getDerivedStateFromProps(nextProps, prevState) {
    if (nextProps.currentRow === prevState.lastRow) {
      return null;
    }
    return {
      lastRow: nextProps.currentRow,
      isScrollingDown: nextProps.currentRow > prevState.lastRow 
    };
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Too vague. Gotcha. I like it.

derivedData: computeDerivedState(
nextProps
),
someMirroredValue:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think especially here this is getting so abstract it's hard to see what's going on. (What is mirroring? what is derived? what you put where and why?)


This would not be safe to do in async mode, because the external callback might get called multiple times for a single update. Instead, the `componentDidUpdate` lifecycle should be used since it is guaranteed to be invoked only once per update:
`embed:update-on-async-rendering/invoking-external-callbacks-after.js`

Copy link
Member

@gaearon gaearon Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add:

Sometimes people use componentWillUpdate out of a misplaced fear that by the time componentDidUpdate fires, it is "too late" to update state of other components. This is not the case. React ensures that any setState calls that happen during componentDidMount and componentDidUpdate are flushed before the user sees the updated UI. In general, it is better to avoid cascading updates like this, but in some cases they are unavoidable (for example, if you need to position a tooltip after measuring the rendered DOM element).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this clarification.

Side note: I wonder how async will impact this. I think it will be important to at least have the option of doing a sync setState flush (for components like RV that require the ability to measure the DOM before doing a meaningful initial render).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the idea is that even in async, setState during commit is sync? At least that's how it works now AFAIK.

This would not be safe to do in async mode, because the external callback might get called multiple times for a single update. Instead, the `componentDidUpdate` lifecycle should be used since it is guaranteed to be invoked only once per update:
`embed:update-on-async-rendering/invoking-external-callbacks-after.js`

## Open source project maintainers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a section before this one. Something like

Other scenarios

While we tried to cover the most common use cases in this post, we recognize that we might have missed some of them. If you are using componentWillMount, componentWillUpdate, or componentWillReceiveProps in ways that aren't covered by this blog post, and aren't sure how to migrate off these legacy lifecycles, please file a new issue against our documentation with your code examples and as much background information as you can provide. We will update this document with new alternative patterns as they come up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it.


We found that asynchronous rendering can help in several ways. For example:

1. As users navigate within an app, newly displayed components often have asynchronous dependencies (including data, images, and code splitting). This leads to a lot of boilerplate code managing data fetching and displaying the loading states. It can also lead to a "cascade of spinners" as the data loads, causing DOM reflows and janky user experience. We'd like to make it easier for product developers to express asynchronous dependencies of components- keeping the old UI "alive" for a certain period while the new UI is not "ready" yet. React could render this new UI in the background and provide a declarative way to show a loading indicator if it takes more than a second.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should - before "keeping" be an em dash?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion:

We'd like to make it easier for product developers to express asynchronous dependencies of components. React could keep the old UI "alive" and interactive for a certain period while the updated UI is not ready yet, and provide a declarative way to show a loading indicator if it takes more than a second.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, probably. My Sublime font renders em dashes and hyphens the same, so I don't notice. I should probably fix this.

isScrollingDown:
nextProps.currentRow >
prevState.lastRow,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very aggressive line breaks make this hard to read. Could we increase the allowed width just a tiny bit?

screen shot 2018-02-07 at 08 12 19

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, man I feel the same about this. I set the line widths I did for Prettier because of mobile layout, but it really makes the desktop layout suffer in cases like this.

I'll play with it a bit to see if I can't find a better solution (maybe smaller font on smaller screens to offset the extra characters a bit).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's probably best done as a follow-up. Even though we don't have many examples, changing the Prettier width affects most of them (which also impacts their relative line highlights).

Maybe I can do it as a stacked PR, on top of this one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we apply this on master once, and later reapply on top of this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I mean, I guess we could. It's probably fine though?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever you prefer :-) I generally prefer doing unrelated changes separately so that they don't depend on each other. This way your final blog post PR won't have to touch every single example. But it's up to you really.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Prettier change only took a minute to do. I just wanted to make sure we were both happy with the resulting impact on mobile. Since it impacted this PR's contents way more than master, I did it on top of this PR.

When I actually merge it, maybe I'll do it directly on master and rebase. I dunno. At the moment, I'm just concerned about getting the content of this PR right. 😄

// highlight-range{1-6}
componentWillMount() {
this.setState({
isScrollingDown: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using the relationship between props.isScrollingDown and props.currentRow in the other example specifically to explain what “derived state” means.

I think using the same names here makes that example less useful. This particular example also doesn’t quite make sense (why would the “last” row be equal to the current row in props?)

I propose each example be different and show a plausible pattern. For example this was one could just be

this.setState({
  currentColor: this.props.defaultColor,
  palette: 'rgb'
});

@bvaughn
Copy link
Contributor Author

bvaughn commented Feb 7, 2018

Merged PR #600 into this branch after rebasing both on master.

Edit: Looks like Netlify has a cashing issue with some of the examples in the latest deploy. Rebuilding (without cache) locally shows the correct, wider line widths.

Edit 2: Pushed an empty commit to restart Netlify and that fixed it.

@@ -0,0 +1,69 @@
---
title: Strict mode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Strict Mode to be consistent with blog post title capitalization elsewhere


For over a year, the React team has been working to implement asynchronous rendering. Last month during his talk at JSConf Iceland, [Dan unveiled some of the exciting new possibilities async rendering unlocks](/blog/2018/03/01/sneak-peek-beyond-react-16.html). Now we'd like to share with you some of the lessons we've learned while working on this feature, and some suggestions we have for preparing your components to be ready for async rendering when it launches.

One of the biggest lesson we've learned is that some of our legacy component lifecycles tend to encourage unsafe coding practices. They are:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"lessons"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I proof read this several times 😭

@@ -35,9 +35,9 @@ Before we begin, here's a quick overview of the lifecycle changes planned for ve

`embed:update-on-async-rendering/new-lifecycles-overview.js`

The new static `getSnapshotBeforeUpdate` lifecycle is invoked after a component is instantiated as well as when it receives new props. It should return an object to update `state`, or `null` to indicate that the new `props` do not require any `state` updates.
The new static `getSnapshotBeforeUpdate` lifecycle is invoked after a component is instantiated as well as when it receives new props. It can return an object to update `state`, or `null` to indicate that the new `props` do not require any `state` updates.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Real stupid question: shouldn't this line be getDerivedStateFromProps ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thanks!

@gaearon
Copy link
Member

gaearon commented Mar 24, 2018

Suggesting some structural tweaks to the first sections: bvaughn#6

`embed:update-on-async-rendering/adding-event-listeners-after.js`

> Note
>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of these notes, should we just plug create-subscription and then show an equivalent example with it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think that's a reasonable change.

Copy link
Contributor Author

@bvaughn bvaughn Mar 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took a stab at this. Let me know what you think?

It's a little awkward since create-subscription won't be released to NPM (and thus won't have a README) until we publish 16.3- so I had to link to the GitHub folder.

I also know there's some concern about over-promoting this package, since there are better solutions for some cases.


> Note
>
> Although the pattern above is slightly more verbose, it has an added benefit of deferring the subscription creation until after the component has rendered, reducing the amount of time in the critical render path. In the near future, React may include more tools to reduce code complexity for data fetching cases like this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it has an added benefit of deferring the subscription creation until after the component has rendered, reducing the amount of time in the critical render path

I don't understand this setence. It seems to imply that componentDidMount doesn't block the initial render (which isn't true, and which I've observed to be a common misconception).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Yeah, that's a good point. I'm not sure why I added that note. 😄

Here is an example of a component that uses the legacy `componentWillReceiveProps` lifecycle to update `state` based on new `props` values:
`embed:update-on-async-rendering/updating-state-from-props-before.js`

Although the above code is not problematic in itself, the `componentWillReceiveProps` lifecycle is often mis-used in ways that _do_ present problems. Because of this, the method has been deprecated.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It hasn't been deprecated yet.


> Note
>
> The [`react-lifecycles-compat`](https://github.com/reactjs/react-lifecycles-compat) polyfill enables this new lifecycle to be used with older versions of React as well. [Learn more about how to use it below.](http://localhost:8000/blog/2018/02/07/update-on-async-rendering.html#open-source-project-maintainers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could be more explicit in this note that this only applies to library authors. Even though we already said it at the top I still anticipate people thinking they need to apply polyfill() in the application code.

@@ -23,7 +23,7 @@ These lifecycle methods have often been misunderstood and subtly misused; furthe

**Note that if you're an application developer, you don't have to do anything about the legacy methods yet. The primary purpose of the upcoming version 16.3 release is to enable open source project maintainers to update their libraries in advance of any deprecation warnings. Those warnings will not be enabled until a future 16.x release.**

At Facebook, we maintain over 50,000 React components, so we're in the same boat as you. We can't rewrite our apps so we will take the gradual migration path together with everyone in the React community.
We maintain over 50,000 React components at Facebook, so we understand that migrations take time. We will take the gradual migration path together with everyone in the React community.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's important to point out our migration path isn't "tell every product team to set everything aside and start fixing their components". People commonly assume that we have this kind of power at Facebook and that product teams just go ahead and spend months catching up with our recommendations (and a "small company wouldn't be able to do this"). So I would prefer if we were very clear in our wording that this is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. Gotcha.

I don't think the previous wording, "we can't rewrite our apps", conveyed that. (It didn't really make sense to me.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure—you're a native speaker so I was just trying to give you the context into what I was trying to express.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I wasn't trying to criticize the wording or say it didn't sound native. I just don't think it conveyed the meaning you are wanting to convey.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

We maintain over 50,000 React components at Facebook, and we don’t plan to rewrite them all immediately. We understand that migrations take time. We will take the gradual migration path along with everyone in the React community.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, I like it

Copy link
Member

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

* `componentWillReceiveProps`
* `componentWillUpdate`

These lifecycle methods have often been misunderstood and subtly misused; furthermore, we anticipate that their potential misuse may be more problematic with async rendering. Because of this, we will be adding an "UNSAFE_" prefix to these lifecycles in an upcoming release.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add:

Here, "unsafe" refers not to security but instead conveys that code using these lifecycles will be more likely to have bugs in future versions of React, especially once async rendering is enabled.

[React follows semantic versioning](/blog/2016/02/19/new-versioning-scheme.html), so this change will be gradual. Our current plan is:

* **16.3**: Introduce aliases for the unsafe lifecycles, `UNSAFE_componentWillMount`, `UNSAFE_componentWillReceiveProps`, and `UNSAFE_componentWillUpdate`. (Both the old lifecycle names and the new aliases will work in this release.)
* **16.x**: Enable deprecation warning for `componentWillMount`, `componentWillReceiveProps`, and `componentWillUpdate`. (Both the old lifecycle names and the new aliases will work in this release, but the old names will log a DEV-mode warning.)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be clearer as "A future 16.x release after 16.3". I kinda read this as meaning that all 16.x are problematic.

const Subscription = createSubscription({
getCurrentValue(sourceProp) {
// Return the current value of the subscription (sourceProp).
// highlight-next-line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the highlighting in this code sample is distracting and I can't tell why some lines are highlighted and not others. I'd just drop all the highlights in this file especially since it isn't really a before/after and ~all the lines are significant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. I was on the fence about this to begin with.

@@ -0,0 +1,36 @@
class ScrollingList extends React.Component {
listRef = null;
prevScrollHeight = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we do previousScrollHeight?


// highlight-range{1-8}
componentDidUpdate(prevProps, prevState, snapshot) {
// If we have a snapshot value, we've just added new items.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment:

// snapshot here will be the value returned from getSnapshotBeforeUpdate.


Fortunately, you do not!

In support of version 16.3, we've also created a new NPM package, [`react-lifecycles-compat`](https://github.com/reactjs/react-lifecycles-compat). This package polyfills components so that the new `getDerivedStateFromProps` lifecycle will also work with older versions of React (0.14.9+).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe: "When React 16.3 is published, we'll also publish a new npm package," since people aren't meant to use it yet?

(also npm should be lowercase https://css-tricks.com/start-sentence-npm/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. I didn't know that (NPM vs npm)

For this reason, the recommended way to add listeners/subscriptions is to use the `componentDidMount` lifecycle:
`embed:update-on-async-rendering/adding-event-listeners-after.js`

Sometimes it is important to update subscriptions in response to property changes. If you're using a library like Redux or MobX, the library's container component should handle this for you. For application authors, we've created a small library, [`create-subscription`](https://github.com/facebook/react/tree/master/packages/create-subscription), to help with this.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"For application authors, we'll created a small library [create-subscription] to help with this and will publish it with React 16.3."


### New lifecycle: `getSnapshotBeforeUpdate`

The new `getSnapshotBeforeUpdate` lifecycle is called right before mutations are made (e.g. before the DOM is updated). The return value for this lifecycle will be passed as the third parameter to `componentDidUpdate`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add: "This lifecycle method usually isn't necessary but is useful in cases like manually preserving scroll position during rerenders." (I'm worried that people will either overuse this or get confused because they can't imagine use cases; I think downplaying it slightly like this will help.)

@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 27, 2018

Thanks Sophie!

@bvaughn bvaughn merged commit 3832311 into reactjs:master Mar 27, 2018
@bvaughn bvaughn deleted the update-on-async-rendering branch March 27, 2018 21:34
@bvaughn
Copy link
Contributor Author

bvaughn commented Mar 27, 2018

A big thank you to everyone who proof read this and shared feedback! ❤️

BetterZxx pushed a commit to BetterZxx/react.dev that referenced this pull request Mar 21, 2023
feat: Sync with reactjs.org @ 4133943

Sync with reactjs.org @ 4133943
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants