diff --git a/STYLE.md b/STYLE.md index ecdcc6aff7e3..642140c65784 100644 --- a/STYLE.md +++ b/STYLE.md @@ -762,6 +762,28 @@ class BComposedComponent extends React.Component } ``` +## isMounted is an Antipattern + +Sometimes we must set React state when a resolved promise is handled. This can cause errors in the JS console because a promise does not have any awareness of whether a component we are setting state on still exists or has unmounted. It may be tempting in these situations to use an instance flag like `this.isComponentMounted` and then set it to `false` in `componentWillUnmount()`. The correct way to handle this is to use a "cancellable" promise. + +```js +componentWillUnmount() { + if (!this.doSomethingPromise) { + return; + } + + this.doSomethingPromise.cancel(); +} + +doSomethingThenSetState() { + this.doSomethingPromise = makeCancellablePromise(this.doSomething()); + this.doSomethingPromise.promise + .then((value) => this.setState({value})); +} +``` + +**Read more:** [https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html](https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html) + ## Composition vs Inheritance From React's documentation - diff --git a/src/components/ImageWithSizeCalculation.js b/src/components/ImageWithSizeCalculation.js index c9dc2acdafb9..4897b4cb1151 100644 --- a/src/components/ImageWithSizeCalculation.js +++ b/src/components/ImageWithSizeCalculation.js @@ -3,6 +3,7 @@ import {Image} from 'react-native'; import PropTypes from 'prop-types'; import Log from '../libs/Log'; import styles from '../styles/styles'; +import makeCancellablePromise from '../libs/MakeCancellablePromise'; const propTypes = { /** Url for image to display */ @@ -28,15 +29,7 @@ const defaultProps = { * it can be appropriately resized. */ class ImageWithSizeCalculation extends PureComponent { - constructor(props) { - super(props); - this.isComponentMounted = false; - } - componentDidMount() { - // If the component unmounts by the time getSize() is finished, it will throw a warning - // So this is to prevent setting state if the component isn't mounted - this.isComponentMounted = true; this.calculateImageSize(); } @@ -49,7 +42,25 @@ class ImageWithSizeCalculation extends PureComponent { } componentWillUnmount() { - this.isComponentMounted = false; + if (!this.getImageSizePromise) { + return; + } + + this.getImageSizePromise.cancel(); + } + + /** + * @param {String} url + * @returns {Promise} + */ + getImageSize(url) { + return new Promise((resolve, reject) => { + Image.getSize(url, (width, height) => { + resolve(width, height); + }, (error) => { + reject(error); + }); + }); } calculateImageSize() { @@ -57,16 +68,19 @@ class ImageWithSizeCalculation extends PureComponent { return; } - Image.getSize(this.props.url, (width, height) => { - if (!width || !height || !this.isComponentMounted) { - // Image didn't load properly or component unmounted before we got the result - return; - } + this.getImageSizePromise = makeCancellablePromise(this.getImageSize(this.props.url)); + this.getImageSizePromise.promise + .then((width, height) => { + if (!width || !height) { + // Image didn't load properly + return; + } - this.props.onMeasure({width, height}); - }, (error) => { - Log.hmmm('Unable to fetch image to calculate size', {error, url: this.props.url}); - }); + this.props.onMeasure({width, height}); + }) + .catch((error) => { + Log.hmmm('Unable to fetch image to calculate size', {error, url: this.props.url}); + }); } render() { diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 57b1c5044597..2a6f98c64f28 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -6,6 +6,7 @@ import Hoverable from '../Hoverable'; import withWindowDimensions from '../withWindowDimensions'; import {propTypes, defaultProps} from './tooltipPropTypes'; import TooltipSense from './TooltipSense'; +import makeCancellablePromise from '../../libs/MakeCancellablePromise'; class Tooltip extends PureComponent { constructor(props) { @@ -36,8 +37,6 @@ class Tooltip extends PureComponent { // The tooltip (popover) itself. this.tooltip = null; - this.isComponentMounted = false; - // Whether the tooltip is first tooltip to activate the TooltipSense this.isTooltipSenseInitiator = false; this.shouldStartShowAnimation = false; @@ -49,36 +48,22 @@ class Tooltip extends PureComponent { this.hideTooltip = this.hideTooltip.bind(this); } - componentDidMount() { - this.isComponentMounted = true; - } - componentDidUpdate(prevProps) { if (this.props.windowWidth === prevProps.windowWidth && this.props.windowHeight === prevProps.windowHeight) { return; } - this.getWrapperPosition() - .then(({x, y}) => this.setStateIfMounted({xOffset: x, yOffset: y})); + this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()); + this.getWrapperPositionPromise.promise + .then(({x, y}) => this.setState({xOffset: x, yOffset: y})); } componentWillUnmount() { - this.isComponentMounted = false; - } - - /** - * Call setState only if this component is mounted. It's necessary to check because we need to call setState - * after an asynchronous `measureInWindow` call, and by the time it completes this component may have unmounted - * and calling setState on an unmounted component results in an error. - * - * @param {Object} newState - */ - setStateIfMounted(newState) { - if (!this.isComponentMounted) { + if (!this.getWrapperPositionPromise) { return; } - this.setState(newState); + this.getWrapperPositionPromise.cancel(); } /** @@ -107,7 +92,7 @@ class Tooltip extends PureComponent { * @param {Object} nativeEvent */ measureTooltip({nativeEvent}) { - this.setStateIfMounted({ + this.setState({ tooltipWidth: nativeEvent.layout.width, tooltipHeight: nativeEvent.layout.height, }); @@ -125,7 +110,8 @@ class Tooltip extends PureComponent { // We have to dynamically calculate the position here as tooltip could have been rendered on some elments // that has changed its position - this.getWrapperPosition() + this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()); + this.getWrapperPositionPromise.promise .then(({ x, y, width, height, }) => { diff --git a/src/libs/MakeCancellablePromise.js b/src/libs/MakeCancellablePromise.js index 256a32ab6fc0..cbe546976114 100644 --- a/src/libs/MakeCancellablePromise.js +++ b/src/libs/MakeCancellablePromise.js @@ -9,8 +9,8 @@ export default function makeCancellablePromise(promise) { let hasCancelled = false; const wrappedPromise = new Promise((resolve, reject) => { - promise.then(val => (hasCancelled ? reject(new Error('Promise was cancelled')) : resolve(val))); - promise.catch(error => (hasCancelled ? reject(new Error('Promise was cancelled')) : reject(error))); + promise.then(val => (hasCancelled ? undefined : resolve(val))); + promise.catch(error => (hasCancelled ? undefined : reject(error))); }); return {