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

Opt into unsafe lifecycle warnings without async tree #12083

Merged
merged 13 commits into from Jan 25, 2018

Conversation

Projects
None yet
5 participants
@bvaughn
Copy link
Contributor

bvaughn commented Jan 23, 2018

Opts a subtree into the same warnings as AsyncComponent does, but does not actually enable async rendering. This same mechanism could be extended for similar warnings in the future (eg compiler, React Fabric).

Following precedent, this component is exposed via React.StrictMode. It is used like a Fragment to wrap an element:

const {StrictMode} = React;

<StrictMode>
  {/* Subtree here */}
</StrictMode>

This branch builds on top of #12060.

@sebmarkbage
Copy link
Member

sebmarkbage left a comment

Duplication is not a good enough reason to introduce an abstraction. Even if file size decreased it can also introduce overhead through other mechanism which is why Prepack tend to work well. This also introduces extra temporary object allocations during initialization. And at the end of the day, I now have to go figure out what configurePrototype actually means instead of just seeing exactly what it does inline in the code when I'm reading it. A lot of the original criticism and the existing problems we have is because of such indirection. Especially when we add more stuff to it. So not sure this is a net positive.

I think we should stop thinking about these changes as enabling async as much as just being future compatible. E.g. a similar opt-in mechanism might be required for the compiler and it'll also require the same deprecations. We might also add more deprecations that only help compilers. Fabric as well. It would be good to have a single story around this instead of having to opt-in for each one.

Maybe we can rename it to something like <EnableFutureCompatibilityChecks /> or something? I don't know.

const pureComponentPrototype = (PureComponent.prototype = new ComponentDummy());
pureComponentPrototype.constructor = PureComponent;
// Avoid an extra prototype jump for these methods.
Object.assign(pureComponentPrototype, Component.prototype);

This comment has been minimized.

@sebmarkbage

sebmarkbage Jan 24, 2018

Member

Are we sure this matters these days in any significant way? Maybe we can just kill this.

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jan 24, 2018

@acdlite Originally we thought that we needed to be able to inherit AsyncComponent instead of wrapping, because at the top level it is tricky since it affects the return value of render. However, if you're upgrading at the top level, you probably want to use the createRoot API instead. Do we still think that we want the inheritance mechanism or should we switch to what context and fragment do?

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Jan 24, 2018

Duplication is not a good enough reason to introduce an abstraction.

The code felt crufty and repetitive before, so I proposed a change. I'll revert it.

Maybe we can rename it to something like <EnableFutureCompatibilityChecks /> or something? I don't know.

Interesting. I like this suggestion. Not sure about that name but I like the suggestion.

@bvaughn bvaughn changed the title New PreAsyncComponent warns about unsafe lifecycles in tree Opt into unsafe lifecycle warnings without async tree Jan 24, 2018

@bvaughn bvaughn force-pushed the bvaughn:12046-part-3 branch from 952b51a to cd4e5a4 Jan 24, 2018

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Jan 24, 2018

I went ahead and made the suggested changes, using EnableFutureCompatibilityChecks for now. I posted a poll internally to see if we can't come up with a better name though.

@gaearon

This comment has been minimized.

Copy link
Member

gaearon commented Jan 24, 2018

To give some background on the abstraction thing, the code feeling crufty/duplicated is how we ended up with a lot of things in the Stack implementation (like "Transactions") that ended up being hard to work with. So these days we try to err on the side of making removal of the code easy rather than making it feel dry.

You know why the abstraction was added and thus know when to remove it. But other people often don’t, and as time goes everybody is afraid to rip out each other’s abstractions, and we end up with overabstracted code that seems to be necessary but no one’s really sure. Hope this makes sense 😛

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Jan 24, 2018

Sure, it makes sense. I didn't feel strongly about the change anyway. I mentioned in the initial PR description that I would be happy to revert it if there were any concerns, for this reason ^

@bvaughn

This comment has been minimized.

Copy link
Contributor Author

bvaughn commented Jan 24, 2018

Based on discussion above and in person (during lunch) I've updated this PR as follows:

  • The new component type is named StrictMode (rather than anything to do with async).
  • The new internal context flag is also named StrictMode.
  • Async subtrees are also StrictMode subtrees and all of the previous async warning checks are now based on this flag instead.
  • The ReactDebugAsyncWarnings helper has been renamed ReactStrictModeWarnings to avoid confusion.
  • A warning message has been updated with a new fb.me link, https://fb.me/react-strict-mode-warnings

@bvaughn bvaughn force-pushed the bvaughn:12046-part-3 branch from 0d626a0 to 1efa2ff Jan 25, 2018

bvaughn added some commits Jan 23, 2018

New PreAsyncComponent warns about unsafe lifecycles in tree
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 #12060
Introduced StrictMode component
Replace previous EnableFutureCompatibilityChecks with a new StrictMode component.

Rename the FutureCompatibilityChecks flag with a StrictMode flag.

Update Async warnings to be based on StrictMode flag instead.

@bvaughn bvaughn force-pushed the bvaughn:12046-part-3 branch from 1efa2ff to 747065b Jan 25, 2018

@sebmarkbage

This comment has been minimized.

Copy link
Member

sebmarkbage commented Jan 25, 2018

What did you think of @tomocchino's ReactDOM.renderStrict(...) proposal to opt in a whole tree?

That would eliminate the need for base class based opt in class Foo extends StrictMode which seems weird to extend a "mode".

@bvaughn bvaughn deleted the bvaughn:12046-part-3 branch Jan 25, 2018

bvaughn added a commit to bvaughn/react that referenced this pull request Jan 25, 2018

Coalesce lifecycle deprecation warnings until the commit phase
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

Coalesce lifecycle deprecation warnings until the commit phase (#12084)
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

This was referenced Mar 30, 2018

@renovate renovate bot referenced this pull request Apr 12, 2018

Merged

Update react monorepo to v16.3.2 #9

bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request Apr 23, 2018

Merge #5
5: Update react monorepo to v16.3.2 r=mythmon a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.2.0` to `16.3.2`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.2.0` to `16.3.2`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.3.0`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1630-March-29-2018)

##### React

* Add a new officially supported context API. ([@&#8203;acdlite] in [#&#8203;11818](`facebook/react#11818))
* Add a new `React.createRef()` API as an ergonomic alternative to callback refs. ([@&#8203;trueadm] in [#&#8203;12162](`facebook/react#12162))
* Add a new `React.forwardRef()` API to let components forward their refs to a child. ([@&#8203;bvaughn] in [#&#8203;12346](`facebook/react#12346))
* Fix a false positive warning in IE11 when using `React.Fragment`. ([@&#8203;XaveScor] in [#&#8203;11823](`facebook/react#11823))
* Replace `React.unstable_AsyncComponent` with `React.unstable_AsyncMode`. ([@&#8203;acdlite] in [#&#8203;12117](`facebook/react#12117))
* Improve the error message when calling `setState()` on an unmounted component. ([@&#8203;sophiebits] in [#&#8203;12347](`facebook/react#12347))
##### React DOM

* Add a new `getDerivedStateFromProps()` lifecycle and `UNSAFE_` aliases for the legacy lifecycles. ([@&#8203;bvaughn] in [#&#8203;12028](`facebook/react#12028))
* Add a new `getSnapshotBeforeUpdate()` lifecycle. ([@&#8203;bvaughn] in [#&#8203;12404](`facebook/react#12404))
* Add a new `<React.StrictMode>` wrapper to help prepare apps for async rendering. ([@&#8203;bvaughn] in [#&#8203;12083](`facebook/react#12083))
* Add support for `onLoad` and `onError` events on the `<link>` tag. ([@&#8203;roderickhsiao] in [#&#8203;11825](`facebook/react#11825))
* Add support for `noModule` boolean attribute on the `<script>` tag. ([@&#8203;aweary] in [#&#8203;11900](`facebook/react#11900))
* Fix minor DOM input bugs in IE and Safari. ([@&#8203;nhunzaker] in [#&#8203;11534](`facebook/react#11534))
* Correctly detect Ctrl + Enter in `onKeyPress` in more browsers. ([@&#8203;nstraub] in [#&#8203;10514](`facebook/react#10514))
* Fix containing elements getting focused on SSR markup mismatch. ([@&#8203;koba04] in [#&#8203;11737](`facebook/react#11737))
* Fix `value` and `defaultValue` to ignore Symbol values. ([@&#8203;nhunzaker] in [#&#8203;11741](`facebook/react#11741))
* Fix refs to class components not getting cleaned up when the attribute is removed. ([@&#8203;bvaughn] in [#&#8203;12178](`facebook/react#12178))
* Fix an IE/Edge issue when rendering inputs into a different window. ([@&#8203;M-ZubairAhmed] in [#&#8203;11870](`facebook/react#11870))
* Throw with a meaningful message if the component runs after jsdom has been destroyed. ([@&#8203;gaearon] in [#&#8203;11677](`facebook/react#11677))
* Don't crash if there is a global variable called `opera` with a `null` value. [@&#8203;alisherdavronov] in [#&#8203;11854](`facebook/react#11854))
* Don't check for old versions of Opera. ([@&#8203;skiritsis] in [#&#8203;11921](`facebook/react#11921))
* Deduplicate warning messages about `<option selected>`. ([@&#8203;watadarkstar] in [#&#8203;11821](`facebook/react#11821))
* Deduplicate warning messages about invalid callback. ([@&#8203;yenshih] in [#&#8203;11833](`facebook/react#11833))
* Deprecate `ReactDOM.unstable_createPortal()` in favor of `ReactDOM.createPortal()`. ([@&#8203;prometheansacrifice] in [#&#8203;11747](`facebook/react#11747))
* Don't emit User Timing entries for context types. ([@&#8203;abhaynikam] in [#&#8203;12250](`facebook/react#12250))
* Improve the error message when context consumer child isn't a function. ([@&#8203;raunofreiberg] in [#&#8203;12267](`facebook/react#12267)) 
* Improve the error message when adding a ref to a functional component. ([@&#8203;skiritsis] in [#&#8203;11782](`facebook/react#11782))
##### React DOM Server

* Prevent an infinite loop when attempting to render portals with SSR. ([@&#8203;gaearon] in [#&#8203;11709](`facebook/react#11709))
* Warn if a class doesn't extend `React.Component`. ([@&#8203;wyze] in [#&#8203;11993](`facebook/react#11993))
* Fix an issue with `this.state` of different components getting mixed up. ([@&#8203;sophiebits] in [#&#8203;12323](`facebook/react#12323))
* Provide a better message when component type is undefined. ([@&#8203;HeroProtagonist] in [#&#8203;11966](`facebook/react#11966))

---

### [`v16.3.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1631-April-3-2018)

##### React

* Fix a false positive warning in IE11 when using `Fragment`. ([@&#8203;heikkilamarko] in [#&#8203;12504](`facebook/react#12504))
* Prefix a private API. ([@&#8203;Andarist] in [#&#8203;12501](`facebook/react#12501))
* Improve the warning when calling `setState()` in constructor. ([@&#8203;gaearon] in [#&#8203;12532](`facebook/react#12532))
##### React DOM

* Fix `getDerivedStateFromProps()` not getting applied in some cases. ([@&#8203;acdlite] in [#&#8203;12528](`facebook/react#12528))
* Fix a performance regression in development mode. ([@&#8203;gaearon] in [#&#8203;12510](`facebook/react#12510))
* Fix error handling bugs in development mode. ([@&#8203;gaearon] and [@&#8203;acdlite] in [#&#8203;12508](`facebook/react#12508))
* Improve user timing API messages for profiling. ([@&#8203;flarnie] in [#&#8203;12384](`facebook/react#12384))
##### Create Subscription

* Set the package version to be in sync with React releases. ([@&#8203;bvaughn] in [#&#8203;12526](`facebook/react#12526))
* Add a peer dependency on React 16.3+. ([@&#8203;NMinhNguyen] in [#&#8203;12496](`facebook/react#12496))

---

### [`v16.3.2`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1632-April-16-2018)

##### React

* Improve the error message when passing `null` or `undefined` to `React.cloneElement`. ([@&#8203;nicolevy] in [#&#8203;12534](`facebook/react#12534))
##### React DOM

* Fix an IE crash in development when using `<StrictMode>`. ([@&#8203;bvaughn] in [#&#8203;12546](`facebook/react#12546))
* Fix labels in User Timing measurements for new component types. ([@&#8203;bvaughn] in [#&#8203;12609](`facebook/react#12609))
* Improve the warning about wrong component type casing. ([@&#8203;nicolevy] in [#&#8203;12533](`facebook/react#12533))
* Improve general performance in development mode. ([@&#8203;gaearon] in [#&#8203;12537](`facebook/react#12537))
* Improve performance of the experimental `unstable_observedBits` API with nesting. ([@&#8203;gaearon] in [#&#8203;12543](`facebook/react#12543))
##### React Test Renderer

* Add a UMD build. ([@&#8203;bvaughn] in [#&#8203;12594](`facebook/react#12594))

---


</details>

# Commits

<details>
<summary>facebook/react</summary>

#### v16.3.0
-   [`c2c3c0c`](facebook/react@c2c3c0c build script to handle react-is (no peer deps) (#&#8203;12471)
-   [`488ad5a`](facebook/react@488ad5a typo in create-subscription readme
-   [`c1b21a7`](facebook/react@c1b21a7 DEV warning if getSnapshotBeforeUpdate is defined as a static method (#&#8203;12475)
-   [`268a3f6`](facebook/react@268a3f6 unstable APIs for async rendering to test renderer (#&#8203;12478)
-   [`c44665e`](facebook/react@c44665e bug when fatal error is thrown as a result of `batch.commit` (#&#8203;12480)
-   [`7a833da`](facebook/react@7a833da) in componentDidMount() should flush synchronously even with createBatch() (#&#8203;12466)
-   [`5855e9f`](facebook/react@5855e9f warning message for setState-on-unmounted (#&#8203;12347)
-   [`15e3dff`](facebook/react@15e3dff#x27;t bail out on referential equality of Consumer&#x27;s props.children function (#&#8203;12470)
-   [`125dd16`](facebook/react@125dd16 user timing to record the timeout deadline with &#x27;waiting&#x27; events (#&#8203;12479)
-   [`96fe3b1`](facebook/react@96fe3b1 React.isValidElementType() (#&#8203;12483)
-   [`53fdc19`](facebook/react@53fdc19 react-is README to show new isValidElementType()
-   [`8650d2a`](facebook/react@8650d2a createRoot for open source builds (#&#8203;12486)
-   [`6294b67`](facebook/react@6294b67 (#&#8203;12487)
-   [`b2379d4`](facebook/react@b2379d4 package versions for release 16.3.0
-   [`9778873`](facebook/react@9778873 dependencies for react-noop-renderer
-   [`8e3d94f`](facebook/react@8e3d94f bundle sizes for 16.3.0 release

#### v16.3.1
-   [`2c3f5fb`](facebook/react@2c3f5fb React 16.3.0 changelog (#&#8203;12488)
-   [`4304475`](facebook/react@4304475 links
-   [`18ba36d`](facebook/react@18ba36d context API in Changelog to &quot;React&quot; section
-   [`59b3905`](facebook/react@59b3905 method name in changelog
-   [`fa8e678`](facebook/react@fa8e678 create-subscription&#x27;s peerDep on react to ^16.3.0 (#&#8203;12496)
-   [`0c80977`](facebook/react@0c80977 React.Fragment props without Map. (#&#8203;12504)
-   [`59dac9d`](facebook/react@59dac9d DEV performance regression by avoiding Object.assign on Fibers (#&#8203;12510)
-   [`6b99c6f`](facebook/react@6b99c6f missing changelog item
-   [`7a27ebd`](facebook/react@7a27ebd user timing to record when we are about to commit (#&#8203;12384)
-   [`4ccf58a`](facebook/react@4ccf58a context stack misalignment caused by error replay (#&#8203;12508)
-   [`6f2ea73`](facebook/react@6f2ea73 throw to separate function so performUnitOfWork does not deopt (#&#8203;12521)
-   [`ba245f6`](facebook/react@ba245f6 _context property on returned ReactContext from createContext - it&#x27;s private (#&#8203;12501)
-   [`eb6e752`](facebook/react@eb6e752 create-subscription package version (#&#8203;12526)
-   [`da4e855`](facebook/react@da4e855 @&#8203;providesModule in www bundles (#&#8203;12529)
-   [`0f2f90b`](https://github.com/facebook/react/commit/0f2f90bd9a9daf241d691bf4af3ea2e3a263c0e3)getDerivedStateFrom{Props,Catch} should update updateQueue.baseState (#&#8203;12528)
-   [`36c2939`](facebook/react@36c2939 not-yet-mounted setState warning (#&#8203;12531)
-   [`a2cc3c3`](facebook/react@a2cc3c3 up: make new warning less wordy (#&#8203;12532)
-   [`2279843`](facebook/react@2279843 yarn.lock file for 16.3.1 release
-   [`787b343`](facebook/react@787b343 package versions for release 16.3.1
-   [`dc05957`](facebook/react@dc05957 bundle sizes for 16.3.1 release
-   [`b15b165`](facebook/react@b15b165 for 16.3.1

#### v16.3.2
-   [`1c2876d`](facebook/react@1c2876d a build step to hoist warning conditions (#&#8203;12537)
-   [`5e3706c`](facebook/react@5e3706c#x27;t render bitmask-bailing consumers even if there&#x27;s a deeper matching child (#&#8203;12543)
-   [`e932e32`](https://github.com/facebook/react/commit/e932e321a88e07935224701bc4580e3dc9889afe)facebook.github.io/react -&gt; reactjs.org (#&#8203;12545)
-   [`d328e36`](facebook/react@d328e36 duplicate typeof check (#&#8203;12541)
-   [`8ec0e4a`](facebook/react@8ec0e4a Array.from() usage (#&#8203;12546)
-   [`27535e7`](facebook/react@27535e7 ReactDOM&#x27;s case warning for html tags (#&#8203;12533)
-   [`7a3416f`](facebook/react@7a3416f component stack from reactTag to React Native renderer (#&#8203;12549)
-   [`cf649b4`](facebook/react@cf649b4 TouchHistoryMath to React Native repo (#&#8203;12557)
-   [`5b16b39`](facebook/react@5b16b39 fix
-   [`6bf2797`](facebook/react@6bf2797 flushSync from React Native (#&#8203;12565)
-   [`bc753a7`](facebook/react@bc753a7 findNodeHandle in Fabric (#&#8203;12573)
-   [`181747a`](https://github.com/facebook/react/commit/181747a6cc25f3020b8561f475eca4ad2824256b)[RN] Move takeSnapshot to RN (#&#8203;12574)
-   [`20c5d97`](facebook/react@20c5d97 consistency in the comment (#&#8203;12579)
-   [`ea37545`](facebook/react@ea37545 be *a* before PlacementAndUpdate (#&#8203;12580)
-   [`76b4ba0`](facebook/react@76b4ba0 error codes for invariants on www (#&#8203;12539)
-   [`8dfb057`](facebook/react@8dfb057 invariant and instead use it from reactProdInvariant (#&#8203;12585)
-   [`f88deda`](facebook/react@f88deda more specific error if passed undefined in React.cloneElement (#&#8203;12534)
-   [`2f7bca0`](facebook/react@2f7bca0 unique reactTags for RN and Fabric (#&#8203;12587)
-   [`933f882`](facebook/react@933f882 ReactNativePropRegistry (#&#8203;12559)
-   [`40d0772`](https://github.com/facebook/react/commit/40d07724fcc801ad69e17b295b68ebea753d5977)[RN] Remove unstable_batchedUpdates and unmountComponentAtNodeAndRemoveContainer from Fabric (#&#8203;12571)
-   [`b6e0512`](facebook/react@b6e0512 eventTypes registry with view configs (#&#8203;12556)
-   [`b99d0b1`](https://github.com/facebook/react/commit/b99d0b14160150c566e091bd10b634beec9a58c3)[RN] Move view config registry to shims (#&#8203;12569)
-   [`725c054`](facebook/react@725c054 findHostInstance and findNodeHandle (#&#8203;12575)
-   [`52afbe0`](facebook/react@52afbe0 needs to be CommonJS
-   [`3eae866`](facebook/react@3eae866 language in error message. (#&#8203;12590)
-   [`b846152`](facebook/react@b846152 UMD build to test renderer package (#&#8203;12594)
-   [`3e9515e`](facebook/react@3e9515e @&#8203;providesModule in www shims
-   [`915bb53`](facebook/react@915bb53 expiration for interactive updates to 150ms in production (#&#8203;12599)
-   [`c27a998`](https://github.com/facebook/react/commit/c27a99812e75e73d9fad88c97ac8b8db452012c1)[Danger] Minor fixes (#&#8203;12606)
-   [`5dfbfe9`](facebook/react@5dfbfe9 debug performance labels for new component types (#&#8203;12609)
-   [`1591c8e`](facebook/react@1591c8e GCC (#&#8203;12618)
-   [`a4cef29`](facebook/react@a4cef29: add regression test for reading ReactCurrentOwner stateNode (#&#8203;12412)
-   [`2e1cc28`](facebook/react@2e1cc28 small typos in create-subscription readme (#&#8203;12399)
-   [`1e97a71`](facebook/react@1e97a71 documentation of the release process (#&#8203;12337)
-   [`66c44a7`](facebook/react@66c44a7 yarn.lock file for 16.3.2 release
-   [`82f67d6`](facebook/react@82f67d6 package versions for release 16.3.2
-   [`6494f6b`](facebook/react@6494f6b error codes for 16.3.2 release
-   [`3232616`](facebook/react@3232616 bundle sizes for 16.3.2 release
-   [`01402f4`](facebook/react@01402f4 16.3.2 changelog (#&#8203;12621)

</details>





---

This PR has been generated by [Renovate Bot](https://renovateapp.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>

@renovate renovate bot referenced this pull request May 1, 2018

Merged

Update react monorepo to v16.3.2 #14

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.