Skip to content

Commit

Permalink
Use getDerivedStateFromProps more simply (#315)
Browse files Browse the repository at this point in the history 
* Update to React@16.4 and add a test for strict mode

This test is a reproduction for #309 and so is currently failing.

There is some complexity in making it work but it reproduces the problem.

* Don't try and avoid updating state

I'm not sure if this is the best solution for #309, as I am not entirely aware of what this code was trying to do, but it *seemed* like it was trying to avoid touching state too much which doesn't seem super useful as there wasn't any complex calculations involved -- and if there was, memoization would be more useful.

In any case it was triggering problems in strict mode which implies it would have broken in later version of React.
  • Loading branch information
@wuweiweiwu
Tom Coleman authored and wuweiweiwu committed Sep 8, 2018
1 parent 8c4fdcc commit 7c22461
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 71 deletions.
4 changes: 2 additions & 2 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,8 @@
"mochify": "^5.8.0",
"mochify-istanbul": "^2.4.2",
"prettier": "^1.13.5",
"react": "^15.6.1",
"react-dom": "^15.6.1",
"react": "^16.4.2",
"react-dom": "^16.4.2",
"rimraf": "^2.6.2",
"rollup": "^0.60.7",
"rollup-plugin-babel": "^3.0.4",
Expand Down
49 changes: 5 additions & 44 deletions src/SplitPane.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ function getDefaultSize(defaultSize, minSize, maxSize, draggedSize) {
}

function removeNullChildren(children) {
return React.Children.toArray(children).filter(c => c);
return React.Children.toArray(children).filter(c => c);
}
class SplitPane extends React.Component {
constructor(props) {
Expand Down Expand Up @@ -65,15 +65,6 @@ class SplitPane extends React.Component {
resized: false,
pane1Size: primary === 'first' ? initialSize : undefined,
pane2Size: primary === 'second' ? initialSize : undefined,

// previous props that we need in static methods
instanceProps: {
primary,
size,
defaultSize,
minSize,
maxSize,
},
};
}

Expand Down Expand Up @@ -214,7 +205,6 @@ class SplitPane extends React.Component {
// TODO: find a more elegant way to fix this. memoize calls to setSize?
// we have to check values since gDSFP is called on every render
static setSize(props, state) {
const { instanceProps } = state;
const newState = {};

const newSize =
Expand All @@ -227,43 +217,14 @@ class SplitPane extends React.Component {
state.draggedSize
);

const defaultSizeChanged =
props.defaultSize !== instanceProps.defaultSize ||
props.minSize !== instanceProps.minSize ||
props.maxSize !== instanceProps.maxSize;

const shouldUpdateSize =
props.size !== undefined
? props.size !== instanceProps.size
: defaultSizeChanged;

if (
props.size !== undefined &&
props.size !== state.draggedSize &&
shouldUpdateSize
) {
if (props.size !== undefined) {
newState.draggedSize = newSize;
}

const isPanel1Primary = props.primary === 'first';

if (shouldUpdateSize || props.primary !== state.instanceProps.primary) {
newState[isPanel1Primary ? 'pane1Size' : 'pane2Size'] = newSize;
}

// unset the size on the non primary panel
if (props.primary !== state.instanceProps.primary) {
newState[isPanel1Primary ? 'pane2Size' : 'pane1Size'] = undefined;
}

// update the values in instanceProps
instanceProps.primary = props.primary;
instanceProps.size = props.size;
instanceProps.defaultSize = props.defaultSize;
instanceProps.minSize = props.minSize;
instanceProps.maxSize = props.maxSize;

newState.instanceProps = instanceProps;
newState[isPanel1Primary ? 'pane1Size' : 'pane2Size'] = newSize;
newState[isPanel1Primary ? 'pane2Size' : 'pane1Size'] = undefined;

return newState;
}
Expand Down Expand Up @@ -296,7 +257,7 @@ class SplitPane extends React.Component {
: resizerClassName;

const notNullChildren = removeNullChildren(children);

const style = Object.assign(
{},
{
Expand Down
5 changes: 5 additions & 0 deletions test/assertions/Asserter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,5 +217,10 @@ export default (jsx, renderToDom = false) => {
expect(primary.props.size).to.equal(undefined);
expect(secondary.props.size).to.equal(50);
},

assertPaneWidthChange(newJsx, expectedWidth, pane = 'first') {
updateComponent(newJsx);
return assertPaneStyles({ width: expectedWidth }, pane);
},
};
};
29 changes: 27 additions & 2 deletions test/default-split-pane-tests.js
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { StrictMode } from 'react';
import chai from 'chai';
import spies from 'chai-spies';

Expand Down Expand Up @@ -145,7 +145,6 @@ describe('Component updates', () => {
<div>two</div>
</SplitPane>
);

it('unsets the width on the non-primary panel when first', () => {
asserter(splitPane1).assertPrimaryPanelChange(
splitPane2,
Expand All @@ -161,4 +160,30 @@ describe('Component updates', () => {
'first'
);
});

it('updates the width of first panel when updating size, in strict mode (#309)', () => {
// For some reason StrictMode renders to null if it is the root of the jsx,
// and we also need the root to be a class-based component. So this is just a complicated
// way of getting around this problem.
class Div extends React.Component {
render() {
return <div>{this.props.children}</div>;
}
}
const paneWithWidth = size => (
<Div>
<StrictMode>
<SplitPane primary="first" size={size}>
<div>one</div>
<div>two</div>
</SplitPane>
</StrictMode>
</Div>
);

asserter(paneWithWidth(100)).assertPaneWidthChange(
paneWithWidth(200),
'200px'
);
});
});
37 changes: 14 additions & 23 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1809,14 +1809,6 @@ create-hmac@^1.1.0, create-hmac@^1.1.2, create-hmac@^1.1.4:
safe-buffer "^5.0.1"
sha.js "^2.4.8"

create-react-class@^15.6.0:
version "15.6.3"
resolved "https://registry.yarnpkg.com/create-react-class/-/create-react-class-15.6.3.tgz#2d73237fb3f970ae6ebe011a9e66f46dbca80036"
dependencies:
fbjs "^0.8.9"
loose-envify "^1.3.1"
object-assign "^4.1.1"

cross-env@^5.2.0:
version "5.2.0"
resolved "https://registry.yarnpkg.com/cross-env/-/cross-env-5.2.0.tgz#6ecd4c015d5773e614039ee529076669b9d126f2"
Expand Down Expand Up @@ -2470,7 +2462,7 @@ fast-levenshtein@~2.0.4:
version "2.0.6"
resolved "https://registry.yarnpkg.com/fast-levenshtein/-/fast-levenshtein-2.0.6.tgz#3d8a5c66883a16a30ca8643e851f19baa7797917"

fbjs@^0.8.9:
fbjs@^0.8.16:
version "0.8.17"
resolved "https://registry.yarnpkg.com/fbjs/-/fbjs-0.8.17.tgz#c4d598ead6949112653d6588b01a5cdcd9f90fdd"
dependencies:
Expand Down Expand Up @@ -4387,7 +4379,7 @@ prompt@~0.2.14:
utile "0.2.x"
winston "0.8.x"

prop-types@^15.5.10, prop-types@^15.5.4, prop-types@^15.6.1:
prop-types@^15.5.10, prop-types@^15.5.4, prop-types@^15.6.0, prop-types@^15.6.1:
version "15.6.2"
resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.6.2.tgz#05d5ca77b4453e985d60fc7ff8c859094a497102"
dependencies:
Expand Down Expand Up @@ -4483,14 +4475,14 @@ rc@^1.1.7:
minimist "^1.2.0"
strip-json-comments "~2.0.1"

react-dom@^15.6.1:
version "15.6.2"
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-15.6.2.tgz#41cfadf693b757faf2708443a1d1fd5a02bef730"
react-dom@^16.4.2:
version "16.4.2"
resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-16.4.2.tgz#4afed569689f2c561d2b8da0b819669c38a0bda4"
dependencies:
fbjs "^0.8.9"
fbjs "^0.8.16"
loose-envify "^1.1.0"
object-assign "^4.1.0"
prop-types "^15.5.10"
object-assign "^4.1.1"
prop-types "^15.6.0"

react-lifecycles-compat@^3.0.4:
version "3.0.4"
Expand All @@ -4502,15 +4494,14 @@ react-style-proptype@^3.0.0:
dependencies:
prop-types "^15.5.4"

react@^15.6.1:
version "15.6.2"
resolved "https://registry.yarnpkg.com/react/-/react-15.6.2.tgz#dba0434ab439cfe82f108f0f511663908179aa72"
react@^16.4.2:
version "16.4.2"
resolved "https://registry.yarnpkg.com/react/-/react-16.4.2.tgz#2cd90154e3a9d9dd8da2991149fdca3c260e129f"
dependencies:
create-react-class "^15.6.0"
fbjs "^0.8.9"
fbjs "^0.8.16"
loose-envify "^1.1.0"
object-assign "^4.1.0"
prop-types "^15.5.10"
object-assign "^4.1.1"
prop-types "^15.6.0"

read-only-stream@^2.0.0:
version "2.0.0"
Expand Down

0 comments on commit 7c22461

Please sign in to comment.