Skip to content

Commit

Permalink
Fix TM stale read bug
Browse files Browse the repository at this point in the history
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.)
  • Loading branch information
chenglou committed Jan 30, 2016
1 parent 972b5c6 commit f20dc1b
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 21 deletions.
58 changes: 38 additions & 20 deletions src/TransitionMotion.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,21 +19,19 @@ import type {

const msPerFrame = 1000 / 60;

type TransitionMotionState = {
currentStyles: Array<PlainStyle>,
currentVelocities: Array<Velocity>,
lastIdealStyles: Array<PlainStyle>,
lastIdealVelocities: Array<Velocity>,
mergedPropsStyles: Array<TransitionStyle>,
};

// the children function & (potential) styles function asks as param an
// Array<TransitionPlainStyle>, 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<TransitionStyle>,
unreadPropStyles: ?Array<TransitionStyle>,
plainStyles: Array<PlainStyle>,
): Array<PlainStyle> {
if (unreadPropStyles == null) {
// no stale props styles value
// $FlowFixMe
return mergedPropsStyles.map((mergedPropsStyle, i) => ({
key: mergedPropsStyle.key,
Expand All @@ -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]};
});
}
Expand Down Expand Up @@ -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<ActualInterpolatingStyleObject>, 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<PlainStyle>,
currentVelocities: Array<Velocity>,
lastIdealStyles: Array<PlainStyle>,
lastIdealVelocities: Array<Velocity>,
// 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<TransitionStyle>,
};

const TransitionMotion = React.createClass({
propTypes: {
defaultStyles: PropTypes.arrayOf(PropTypes.shape({
Expand All @@ -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,
};
},
Expand Down Expand Up @@ -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;
Expand All @@ -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]};
Expand All @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion src/stripStyle.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down

0 comments on commit f20dc1b

Please sign in to comment.