Warn if unsafe lifecycle methods are found in an async subtree #12060

bvaughn · 2018-01-20T03:58:31Z

Part two of #12046

This PR identifies class-components using unsafe lifecycles (componenWillMount, componenWillReceiveProps, componenWillUpdate) during the begin phase, and prints warnings about them (and with the location of the async subtree they belong to) during the commit phase. In the event of an error, pending warnings are discarded by performFailedUnitOfWork.

Example warning

A warning for a particular async subtree will look like this:

Unsafe lifecycle methods were found within the following async tree:
in AsyncRoot (at AsyncRoot.js:260)
in div (at SyncRoot.js:259)
in SyncRoot (at SyncRoot.js:306)

componentWillMount: Please update the following components to use componentDidMount instead: Foo, Bar

componentWillReceiveProps: Please update the following components to use static getDerivedDataFromState instead: Foo

componentWillUpdate: Please update the following components to use componentDidUpdate instead: Foo, Bar, Baz

Learn more about this warning here:
https://fb.me/react-async-component-lifecycle-hooks

How are warnings grouped?

I put a lot of consideration into how to group/coalesce the warnings, as well as how to dedup them. I landed on the following:

Grouped by async subtree, and then printed by lifecycle. I think this grouping makes as much sense as anything else, in terms of how you would approach tracking down and fixing the lifecycles. (Although I'm happy to tweak the wording and format if anyone has suggestions.)
Deduped per class component type (by way of an dev-only, expando attribute). This was a nice tradeoff between uniqueness concerns (mentioned below) and implementation complexity.

Alternatives considered

I considered different deduping strategies but this felt the most pragmatic. Other considerations were:

Per async-subtree (aka per component stack). Major drawback: Components not rendered initially (due to conditional logic, blockers, etc) would never get reported.
Per unsafe component name: Major drawback: Component names might be vague (eg "Text") and might have collisions- which could lead to several rounds of fixings warnings about a component with a given name.
No deduping: Major drawback: Lots of irritating console noise.

Open Questions

Is the way I'm flushing or discarding pendingWarningsMap safe? Are there cases (like interruptions) that it won't handle well? (cc @acdlite)
In the event of an error, warnings may be split into multiple messages since the code below the boundary is committed in a different pass. I think this is okay since it's not a common case.

pull-bot · 2018-01-23T00:42:59Z

Details of bundled changes.

Comparing: 4ca7855...97e2820

react-native-renderer

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
ReactNativeRenderer-dev.js	+1%	+1%	410.59 KB	414.77 KB	90.63 KB	91.57 KB	RN_DEV
ReactFabric-dev.js	+Infinity%	+Infinity%	0 B	406.35 KB	0 B	89.82 KB	RN_DEV
ReactFabric-prod.js	🔺+Infinity%	🔺+Infinity%	0 B	191.24 KB	0 B	33.62 KB	RN_PROD

Generated by 🚫 dangerJS

bvaughn · 2018-01-23T03:21:33Z

Oh shit, looks like I've increased the size of React Fabric by infinity 😱😱😱

gaearon · 2018-01-23T11:42:13Z

Lol, sorry, the detection is a bit wrong. I’m actually surprised why it still runs because I thought I disabled it in 04d8fec. Maybe your PR is based on an earlier commit.

bvaughn · 2018-01-23T11:57:40Z

Note to self for when I'm more awake: Do I need to reset pending warnings in the event of an error or interruption? Or is it okay to just warn on the next complete phrase, since the unsafe methods still exist in the source?

May best to explicitly reset when starting a new begin phase just in case? Will want to add a test case for this if so.

Edit I think it is okay to still warn in this case. I am open to discussion here though if others disagree.

Unsafe async warnings are now grouped by their parent async trees (whihc can be identified via component stacks included in the message). We also coalesce warnings and print during the complete phase. Warnings are deduped per component type, after much consideration.

bvaughn · 2018-01-23T16:05:34Z

Rebased~

This causes too much noise in the log. Also added/hardened tests around this case.

This is based on a suggestion from Jed Watson that the UNSAFE_* prefix in the warning message was a little confusing

bvaughn · 2018-01-23T18:29:27Z

Regarding an initial question in the PR description:

Is the way I'm removing warnings from pendingWarningsMap safe or will it leak? Is there somewhere I should completely reset the Map to avoid potentially leaking (like performFailedUnitOfWork or renderRoot)?

I think I picked a safer approach in a1b5f0f, but if this is incorrect, let me know.

Works like AsyncComponent except that it does not actually enable async rendering. This component is exposed via React.unstable_PreAsyncComponent (following precedent). I also tidied up ReactBaseClass a little because the duplication was bothering me. I will revert this if there's any concern. This branch is stacked on top of 12046-part-2 and PR facebook#12060

sebmarkbage

I think we'll need a URL to a more descriptive doc where we can document all the patterns to upgrade to.

sebmarkbage · 2018-01-23T21:33:21Z

packages/react-reconciler/src/ReactDebugAsyncWarnings.js

+    // are often vague and are likely to collide between 3rd party libraries.
+    // An expand property is probably okay to use here since it's DEV-only,
+    // and will only be set in the event of serious warnings.
+    if (fiber.type[DID_WARN_KEY] === true) {


We are using a Map in here anyway so why don't we just use a Set for this? (To ensure that it is safe on frozen objects.)

bvaughn · 2018-01-23T21:36:03Z

I think we'll need a URL to a more descriptive doc where we can document all the patterns to upgrade to.

Yes, totally. This is why I used an fb.me link. We can update it once we write a blog post or documentation.

Works like AsyncComponent except that it does not actually enable async rendering. This component is exposed via React.unstable_PreAsyncComponent (following precedent). I also tidied up ReactBaseClass a little because the duplication was bothering me. I will revert this if there's any concern. This branch is stacked on top of 12046-part-2 and PR facebook#12060

Builds on top of PR facebook#12083 and resolves issue facebook#12044. Coalesces deprecation warnings until the commit phase. This proposal extends the utility introduced in facebook#12060 to also coalesce deprecation warnings. New warning format will look like this: > componentWillMount is deprecated and will be removed in the next major version. Use componentDidMount instead. As a temporary workaround, you can rename to UNSAFE_componentWillMount. > > Please update the following components: Foo, Bar > > Learn more about this warning here: > https://fb.me/react-async-component-lifecycle-hooks

Builds on top of PR #12083 and resolves issue #12044. Coalesces deprecation warnings until the commit phase. This proposal extends the utility introduced in #12060 to also coalesce deprecation warnings. New warning format will look like this: > componentWillMount is deprecated and will be removed in the next major version. Use componentDidMount instead. As a temporary workaround, you can rename to UNSAFE_componentWillMount. > > Please update the following components: Foo, Bar > > Learn more about this warning here: > https://fb.me/react-async-component-lifecycle-hooks

bvaughn mentioned this pull request Jan 20, 2018

Update debugRenderPhaseSideEffects behavior #12046

Closed

2 tasks

facebook-github-bot added the CLA Signed label Jan 20, 2018

mihalskiy approved these changes Jan 20, 2018

View reviewed changes

bvaughn requested review from gaearon and acdlite January 23, 2018 00:39

bvaughn added 4 commits January 23, 2018 08:04

Warn if unsafe lifecycle methods are found in an async subtree

e41cbae

Made unsafe lifecycle warnings more succinct

3bd6529

Added an additional test

05aff4d

bvaughn force-pushed the 12046-part-2 branch from 97e2820 to 05aff4d Compare January 23, 2018 16:05

bvaughn added 3 commits January 23, 2018 08:48

Don't prematurely log warnings before completing async root.

2d8730c

This causes too much noise in the log. Also added/hardened tests around this case.

Changed unsafe warning method to improove clarity.

d90987f

This is based on a suggestion from Jed Watson that the UNSAFE_* prefix in the warning message was a little confusing

Improved flushing of pending warnings to avoid leaks

a1b5f0f

bvaughn mentioned this pull request Jan 23, 2018

Opt into unsafe lifecycle warnings without async tree #12083

Merged

sebmarkbage approved these changes Jan 23, 2018

View reviewed changes

sebmarkbage reviewed Jan 23, 2018

View reviewed changes

Replaced DID_WARN expando attribute with a Set

c1b17b3

bvaughn merged commit cba51ba into facebook:master Jan 23, 2018

bvaughn deleted the 12046-part-2 branch January 23, 2018 22:01

bvaughn mentioned this pull request Jan 23, 2018

Coalesce lifecycle deprecation warnings until the commit phase #12084

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn if unsafe lifecycle methods are found in an async subtree #12060

Warn if unsafe lifecycle methods are found in an async subtree #12060

bvaughn commented Jan 20, 2018 •

edited

Loading

pull-bot commented Jan 23, 2018 •

edited

Loading

react-native-renderer

bvaughn commented Jan 23, 2018

gaearon commented Jan 23, 2018

bvaughn commented Jan 23, 2018 •

edited

Loading

bvaughn commented Jan 23, 2018

bvaughn commented Jan 23, 2018 •

edited

Loading

sebmarkbage left a comment

sebmarkbage Jan 23, 2018 •

edited

Loading

bvaughn commented Jan 23, 2018

Warn if unsafe lifecycle methods are found in an async subtree #12060

Warn if unsafe lifecycle methods are found in an async subtree #12060

Conversation

bvaughn commented Jan 20, 2018 • edited Loading

Example warning

How are warnings grouped?

Alternatives considered

Open Questions

pull-bot commented Jan 23, 2018 • edited Loading

react-native-renderer

bvaughn commented Jan 23, 2018

gaearon commented Jan 23, 2018

bvaughn commented Jan 23, 2018 • edited Loading

bvaughn commented Jan 23, 2018

bvaughn commented Jan 23, 2018 • edited Loading

sebmarkbage left a comment

Choose a reason for hiding this comment

sebmarkbage Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

bvaughn commented Jan 23, 2018

bvaughn commented Jan 20, 2018 •

edited

Loading

pull-bot commented Jan 23, 2018 •

edited

Loading

bvaughn commented Jan 23, 2018 •

edited

Loading

bvaughn commented Jan 23, 2018 •

edited

Loading

sebmarkbage Jan 23, 2018 •

edited

Loading