From 5a77746c879268277b6efd2290614d42725d8d5b Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 19 Jan 2022 08:28:11 -1000 Subject: [PATCH 1/4] Use makeCancellablePromise instead of isComponentMounted --- src/components/ImageWithSizeCalculation.js | 49 ++++++++++++++-------- src/components/Tooltip/index.js | 30 ++++--------- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/src/components/ImageWithSizeCalculation.js b/src/components/ImageWithSizeCalculation.js index c9dc2acdafb92..97a84829ebfed 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,18 @@ 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)) + .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 57b1c50445970..9dbfb93904e73 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,21 @@ 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()) + .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 +91,7 @@ class Tooltip extends PureComponent { * @param {Object} nativeEvent */ measureTooltip({nativeEvent}) { - this.setStateIfMounted({ + this.setState({ tooltipWidth: nativeEvent.layout.width, tooltipHeight: nativeEvent.layout.height, }); @@ -125,7 +109,7 @@ 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()) .then(({ x, y, width, height, }) => { From 17b4cdc6e424848162ca0e4215e4da427f1a9100 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 19 Jan 2022 08:35:37 -1000 Subject: [PATCH 2/4] Add to style guide --- STYLE.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/STYLE.md b/STYLE.md index ecdcc6aff7e3a..66ded9fc3e20c 100644 --- a/STYLE.md +++ b/STYLE.md @@ -762,6 +762,27 @@ 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()) + .then((value) => this.setState({value})); +} +``` + +**Read more:** https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html + ## Composition vs Inheritance From React's documentation - From 534483563ced5287cea68b38fd0b13027f371a66 Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 19 Jan 2022 08:38:30 -1000 Subject: [PATCH 3/4] make link --- STYLE.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/STYLE.md b/STYLE.md index 66ded9fc3e20c..0f186de81f0e9 100644 --- a/STYLE.md +++ b/STYLE.md @@ -781,7 +781,7 @@ doSomethingThenSetState() { } ``` -**Read more:** https://reactjs.org/blog/2015/12/16/ismounted-antipattern.html +**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 c7fb490bc66093dab835963ecdfd769ff3d9ad5f Mon Sep 17 00:00:00 2001 From: Marc Glasser Date: Wed, 19 Jan 2022 09:33:30 -1000 Subject: [PATCH 4/4] Stop throwing in makeCancellablePromise --- STYLE.md | 3 ++- src/components/ImageWithSizeCalculation.js | 3 ++- src/components/Tooltip/index.js | 6 ++++-- src/libs/MakeCancellablePromise.js | 4 ++-- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/STYLE.md b/STYLE.md index 0f186de81f0e9..642140c657844 100644 --- a/STYLE.md +++ b/STYLE.md @@ -776,7 +776,8 @@ componentWillUnmount() { } doSomethingThenSetState() { - this.doSomethingPromise = makeCancellablePromise(this.doSomething()) + this.doSomethingPromise = makeCancellablePromise(this.doSomething()); + this.doSomethingPromise.promise .then((value) => this.setState({value})); } ``` diff --git a/src/components/ImageWithSizeCalculation.js b/src/components/ImageWithSizeCalculation.js index 97a84829ebfed..4897b4cb11510 100644 --- a/src/components/ImageWithSizeCalculation.js +++ b/src/components/ImageWithSizeCalculation.js @@ -68,7 +68,8 @@ class ImageWithSizeCalculation extends PureComponent { return; } - this.getImageSizePromise = makeCancellablePromise(this.getImageSize(this.props.url)) + this.getImageSizePromise = makeCancellablePromise(this.getImageSize(this.props.url)); + this.getImageSizePromise.promise .then((width, height) => { if (!width || !height) { // Image didn't load properly diff --git a/src/components/Tooltip/index.js b/src/components/Tooltip/index.js index 9dbfb93904e73..2a6f98c64f287 100644 --- a/src/components/Tooltip/index.js +++ b/src/components/Tooltip/index.js @@ -53,7 +53,8 @@ class Tooltip extends PureComponent { return; } - this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()) + this.getWrapperPositionPromise = makeCancellablePromise(this.getWrapperPosition()); + this.getWrapperPositionPromise.promise .then(({x, y}) => this.setState({xOffset: x, yOffset: y})); } @@ -109,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.getWrapperPositionPromise = makeCancellablePromise(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 256a32ab6fc06..cbe5469761147 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 {