From f20dc1b9c8de7b387927b24afdb73e0a5ea0d0a6 Mon Sep 17 00:00:00 2001 From: Cheng Lou Date: Sat, 30 Jan 2016 05:04:19 -0500 Subject: [PATCH] Fix TM stale read bug Repro: go to todomvc, check an item, type something that'd filter it out, see that for the brief unmounting animation the check mark is gone (stale `isDone: false`). This fixes that by removing the dirty check in `clearUnreadPropStyle`. We now setState every time this is called (every willUpdate). This is not underperformant as willUpdate will batch that setState without triggering an extra render. There are a bunch of nuances in this. I knew the staleness was a problem but kept the dirty check anyway because fwiw `rehydrateStyles` always picked the freshest `data` (the field in `TransitionStyle`), and the dirty data is never read anywhere. Apparently I was wrong. This piece is tricky and should be revisited/commented better. (No test yet.) --- src/TransitionMotion.js | 58 +++++++++++++++++++++++++++-------------- src/stripStyle.js | 2 +- 2 files changed, 39 insertions(+), 21 deletions(-) diff --git a/src/TransitionMotion.js b/src/TransitionMotion.js index a9cfd9da..0b8257fc 100644 --- a/src/TransitionMotion.js +++ b/src/TransitionMotion.js @@ -19,21 +19,19 @@ import type { const msPerFrame = 1000 / 60; -type TransitionMotionState = { - currentStyles: Array, - currentVelocities: Array, - lastIdealStyles: Array, - lastIdealVelocities: Array, - mergedPropsStyles: Array, -}; - +// the children function & (potential) styles function asks as param an +// Array, where each TransitionPlainStyle is of the format +// {key: string, data?: any, style: PlainStyle}. However, the way we keep +// internal states doesn't contain such a data structure (check the state and +// TransitionMotionState). So when children function and others ask for such +// data we need to generate them on the fly by combining mergedPropsStyles and +// currentStyles/lastIdealStyles function rehydrateStyles( mergedPropsStyles: Array, unreadPropStyles: ?Array, plainStyles: Array, ): Array { if (unreadPropStyles == null) { - // no stale props styles value // $FlowFixMe return mergedPropsStyles.map((mergedPropsStyle, i) => ({ key: mergedPropsStyle.key, @@ -47,12 +45,14 @@ function rehydrateStyles( // $FlowFixMe if (unreadPropStyles[j].key === mergedPropsStyle.key) { return { + // $FlowFixMe key: unreadPropStyles[j].key, data: unreadPropStyles[j].data, style: plainStyles[i], }; } } + // $FlowFixMe return {key: mergedPropsStyle.key, data: mergedPropsStyle.data, style: plainStyles[i]}; }); } @@ -165,6 +165,23 @@ function mergeAndSync( return [newMergedPropsStyles, newCurrentStyles, newCurrentVelocities, newLastIdealStyles, newLastIdealVelocities]; } +type TransitionMotionState = { + // list of styles, each containing interpolating values. Part of what's passed + // to children function. Notice that this is + // Array, without the wrapper that is {key: ..., + // data: ... style: ActualInterpolatingStyleObject}. Only mergedPropsStyles + // contains the key & data info (so that we only have a single source of truth + // for these, and to save space). Check the comment for `rehydrateStyles` to + // see how we regenerate the entirety of what's passed to children function + currentStyles: Array, + currentVelocities: Array, + lastIdealStyles: Array, + lastIdealVelocities: Array, + // the array that keeps track of currently rendered stuff! Including stuff + // that you've unmounted but that's still animating. This is where it lives + mergedPropsStyles: Array, +}; + const TransitionMotion = React.createClass({ propTypes: { defaultStyles: PropTypes.arrayOf(PropTypes.shape({ @@ -191,6 +208,8 @@ const TransitionMotion = React.createClass({ getDefaultProps(): {willEnter: WillEnter, willLeave: WillLeave} { return { willEnter: styleThatEntered => stripStyle(styleThatEntered.style), + // recall: returning null makes the current unmounting TransitionStyle + // disappear immediately willLeave: () => null, }; }, @@ -272,7 +291,6 @@ const TransitionMotion = React.createClass({ this.state.lastIdealVelocities, ); - let someDirty = false; for (let i = 0; i < unreadPropStyles.length; i++) { const unreadPropStyle = unreadPropStyles[i].style; let dirty = false; @@ -286,7 +304,6 @@ const TransitionMotion = React.createClass({ if (typeof styleValue === 'number') { if (!dirty) { dirty = true; - someDirty = true; currentStyles[i] = {...currentStyles[i]}; currentVelocities[i] = {...currentVelocities[i]}; lastIdealStyles[i] = {...lastIdealStyles[i]}; @@ -306,15 +323,16 @@ const TransitionMotion = React.createClass({ } } - if (someDirty) { - this.setState({ - currentStyles, - currentVelocities, - mergedPropsStyles, - lastIdealStyles, - lastIdealVelocities, - }); - } + // unlike the other 2 components, we can't detect staleness and optionally + // opt out of setState here. each style object's data might contain new + // stuff we're not/cannot compare + this.setState({ + currentStyles, + currentVelocities, + mergedPropsStyles, + lastIdealStyles, + lastIdealVelocities, + }); }, startAnimationIfNecessary(): void { diff --git a/src/stripStyle.js b/src/stripStyle.js index 5503276e..ebc90065 100644 --- a/src/stripStyle.js +++ b/src/stripStyle.js @@ -1,6 +1,6 @@ /* @flow */ // turn {x: {val: 1, stiffness: 1, damping: 2}, y: 2} generated by -// `{x: spring(1, [1, 2]), y: 2}` into {x: 1, y: 2} +// `{x: spring(1, {stiffness: 1, damping: 2}), y: 2}` into {x: 1, y: 2} import type {Style, PlainStyle} from './Types';