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

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

Merged
merged 8 commits into from
Jan 23, 2018

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 20, 2018

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
Copy link

pull-bot commented Jan 23, 2018

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
Copy link
Contributor Author

bvaughn commented Jan 23, 2018

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

screen shot 2018-01-22 at 7 21 00 pm

@gaearon
Copy link
Collaborator

gaearon commented Jan 23, 2018

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
Copy link
Contributor Author

bvaughn commented Jan 23, 2018

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
Copy link
Contributor Author

bvaughn commented Jan 23, 2018

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
Copy link
Contributor Author

bvaughn commented Jan 23, 2018

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.

bvaughn added a commit to bvaughn/react that referenced this pull request Jan 23, 2018
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
Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

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

// 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) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Jan 23, 2018

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

bvaughn commented Jan 23, 2018

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.

@bvaughn bvaughn merged commit cba51ba into facebook:master Jan 23, 2018
@bvaughn bvaughn deleted the 12046-part-2 branch January 23, 2018 22:01
bvaughn added a commit to bvaughn/react that referenced this pull request Jan 23, 2018
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
bvaughn added a commit to bvaughn/react that referenced this pull request Jan 23, 2018
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
bvaughn added a commit to bvaughn/react that referenced this pull request Jan 25, 2018
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
bvaughn added a commit to bvaughn/react that referenced this pull request Jan 25, 2018
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
bvaughn added a commit that referenced this pull request Jan 25, 2018
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
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.

6 participants