From 1ac7fad60880648bf0d6a9e545f31149066bcda4 Mon Sep 17 00:00:00 2001 From: Colby Rabideau Date: Mon, 6 Mar 2017 10:56:53 -0500 Subject: [PATCH] Clear dispatch token It seems that in certain versions of React it was possible for `componentWillMount` to be called more than once. https://github.com/facebook/react/pull/6613 If that happened to a `connect`ed component, it would cause `dispatcher.unregister` to be called again with a token that was already unregistered causing an exception. This `null`s the token field so unregister can only be called once for a given token. --- src/dependencies/StoreDependencyMixin.js | 53 +++++++++--------------- src/dependencies/connect.js | 44 ++++++++------------ 2 files changed, 37 insertions(+), 60 deletions(-) diff --git a/src/dependencies/StoreDependencyMixin.js b/src/dependencies/StoreDependencyMixin.js index c2e6657..8740360 100644 --- a/src/dependencies/StoreDependencyMixin.js +++ b/src/dependencies/StoreDependencyMixin.js @@ -16,20 +16,17 @@ import { enforceDispatcher } from '../dispatcher/DispatcherInterface'; import invariant from 'invariant'; type ReactMixin = { - __dispatchToken?: string; - propTypes?: Object; - componentWillMount: Function; - componentWillReceiveProps: Function; - componentDidUpdate?: Function; - componentWillUnmount: Function; -} + __dispatchToken?: string, + propTypes?: Object, + componentWillMount: Function, + componentWillReceiveProps: Function, + componentDidUpdate?: Function, + componentWillUnmount: Function, +}; -function onlyStoreStateChanged(dependencies, state, prevState): bool { +function onlyStoreStateChanged(dependencies, state, prevState): boolean { for (const field in state) { - if ( - !dependencies.hasOwnProperty(field) && - state[field] !== prevState[field] - ) { + if (!dependencies.hasOwnProperty(field) && state[field] !== prevState[field]) { return false; } } @@ -38,7 +35,7 @@ function onlyStoreStateChanged(dependencies, state, prevState): bool { export default function StoreDependencyMixin( dependencies: DependencyMap, - dispatcher: ?Dispatcher = DispatcherInstance.get() + dispatcher: ?Dispatcher = DispatcherInstance.get(), ): ReactMixin { enforceDispatcher(dispatcher); @@ -53,36 +50,28 @@ export default function StoreDependencyMixin( invariant( this.__dispatchToken === undefined, 'Only one `StoreDependencyMixin` may be registered on `%s`', - this.constructor.displayName + this.constructor.displayName, ); if (dispatcher) { this.__dispatchToken = dispatcher.register( - handleDispatch.bind( - null, - dispatcher, - dependencyIndex, - (entry) => { - this.setState( - calculateForDispatch(dependencies, entry, this.props, this.state) - ); - } - ) + handleDispatch.bind(null, dispatcher, dependencyIndex, entry => { + this.setState(calculateForDispatch(dependencies, entry, this.props, this.state)); + }), ); } }, componentWillReceiveProps(nextProps): void { - this.setState( - calculateForPropsChange(dependencies, nextProps, this.state) - ); + this.setState(calculateForPropsChange(dependencies, nextProps, this.state)); }, componentWillUnmount(): void { - if (dispatcher && this.__dispatchToken) { - dispatcher.unregister(this.__dispatchToken); + const dispatchToken = this.__dispatchToken; + if (dispatcher && dispatchToken) { + this.__dispatchToken = null; + dispatcher.unregister(dispatchToken); } }, - }; if (dependenciesUseState(dependencies)) { @@ -90,9 +79,7 @@ export default function StoreDependencyMixin( if (onlyStoreStateChanged(dependencies, this.state, prevState)) { return; } - this.setState( - calculateForStateChange(dependencies, this.props, this.state) - ); + this.setState(calculateForStateChange(dependencies, this.props, this.state)); }; } diff --git a/src/dependencies/connect.js b/src/dependencies/connect.js index 80d2976..62220a6 100644 --- a/src/dependencies/connect.js +++ b/src/dependencies/connect.js @@ -1,23 +1,23 @@ /* @flow */ -import type {DependencyIndexEntry, DependencyMap} from './DependencyMap'; -import type {Dispatcher} from 'flux'; +import type { DependencyIndexEntry, DependencyMap } from './DependencyMap'; +import type { Dispatcher } from 'flux'; import { calculateInitial, calculateForDispatch, calculateForPropsChange, dependencyPropTypes, - makeDependencyIndex + makeDependencyIndex, } from '../dependencies/DependencyMap'; -import {handleDispatch} from './Dispatch'; -import {get as getDispatcherInstance} from '../dispatcher/DispatcherInstance'; -import {enforceDispatcher} from '../dispatcher/DispatcherInterface'; -import React, {Component} from 'react'; +import { handleDispatch } from './Dispatch'; +import { get as getDispatcherInstance } from '../dispatcher/DispatcherInstance'; +import { enforceDispatcher } from '../dispatcher/DispatcherInterface'; +import React, { Component } from 'react'; function transferStaticProperties( fromClass: Object, // By setting the type to Object, I'm doing a little dance around the type // checker... I fully expect this to break after a future flow upgrade. - toClass: Object + toClass: Object, ) { Object.keys(fromClass).forEach(staticField => { toClass[staticField] = fromClass[staticField]; @@ -26,7 +26,7 @@ function transferStaticProperties( export default function connect( dependencies: DependencyMap, - dispatcher: ?Dispatcher = getDispatcherInstance() + dispatcher: ?Dispatcher = getDispatcherInstance(), ): Function { enforceDispatcher(dispatcher); @@ -47,33 +47,26 @@ export default function connect( componentWillMount() { if (dispatcher) { this.dispatchToken = dispatcher.register( - handleDispatch.bind( - null, - dispatcher, - dependencyIndex, - this.handleDispatch.bind(this) - ) + handleDispatch.bind(null, dispatcher, dependencyIndex, this.handleDispatch.bind(this)), ); } this.setState(calculateInitial(dependencies, this.props, this.state)); } componentWillReceiveProps(nextProps: Object): void { - this.setState( - calculateForPropsChange(dependencies, nextProps, this.state) - ); + this.setState(calculateForPropsChange(dependencies, nextProps, this.state)); } componentWillUnmount(): void { - if (dispatcher && this.dispatchToken) { - dispatcher.unregister(this.dispatchToken); + const dispatchToken = this.dispatchToken; + if (dispatcher && dispatchToken) { + this.dispatchToken = null; + dispatcher.unregister(dispatchToken); } } handleDispatch(entry: DependencyIndexEntry) { - this.setState( - calculateForDispatch(dependencies, entry, this.props, this.state) - ); + this.setState(calculateForDispatch(dependencies, entry, this.props, this.state)); } render() { @@ -83,10 +76,7 @@ export default function connect( transferStaticProperties(BaseComponent, ConnectedComponent); ConnectedComponent.displayName = `Connected(${BaseComponent.displayName})`; - ConnectedComponent.propTypes = dependencyPropTypes( - dependencies, - BaseComponent.propTypes - ); + ConnectedComponent.propTypes = dependencyPropTypes(dependencies, BaseComponent.propTypes); return ConnectedComponent; };