Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions STYLE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 -
Expand Down
50 changes: 32 additions & 18 deletions src/components/ImageWithSizeCalculation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand All @@ -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();
}

Expand All @@ -49,24 +42,45 @@ 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() {
if (!this.props.url) {
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() {
Expand Down
32 changes: 9 additions & 23 deletions src/components/Tooltip/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
Expand All @@ -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();
}

/**
Expand Down Expand Up @@ -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,
});
Expand All @@ -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,
}) => {
Expand Down
4 changes: 2 additions & 2 deletions src/libs/MakeCancellablePromise.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down