Skip to content

Remove UNSAFE_componentWillReceiveProps from react-json-tree #1049

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

Remove UNSAFE_componentWillReceiveProps from react-json-tree #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f70f183
Remove UNSAFE_componentWillReceiveProps from JSONTree
defunctzombie Jan 23, 2022
68dba97
Remove UNSAFE_componentWillReceiveProps from JSONNestedNode
defunctzombie Jan 23, 2022
9a8a6e9
default export
defunctzombie Jan 24, 2022
6b4da1b
Merge branch 'main' into roman/react-json-tree-remove-unsafe
defunctzombie Jan 24, 2022
9c00bdd
Merge branch 'main' into roman/react-json-tree-remove-unsafe
defunctzombie Jan 30, 2022
ed9cb23
update prop types to allow for optional _circularCache_ and _expandab…
defunctzombie Jan 31, 2022
03a8af7
Merge branch 'main' into roman/react-json-tree-remove-unsafe
defunctzombie Jan 31, 2022
ffcbe5d
fix root props
defunctzombie Jan 31, 2022
043682d
set circularCache default in renderChildNodes
defunctzombie Jan 31, 2022
cf66127
Merge branch 'main' into roman/react-json-tree-remove-unsafe
defunctzombie Feb 5, 2022
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
247 changes: 108 additions & 139 deletions packages/react-json-tree/src/JSONNestedNode.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { useCallback, useEffect, useRef, useState } from 'react';
import PropTypes from 'prop-types';
import JSONArrow from './JSONArrow';
import getCollectionEntries from './getCollectionEntries';
Expand Down Expand Up @@ -42,7 +42,7 @@ function renderChildNodes(
nodeType,
data,
collectionLimit,
circularCache,
circularCache = [],
keyPath,
postprocessValue,
sortObjectKeys,
Expand Down Expand Up @@ -94,147 +94,116 @@ interface Props extends CircularPropsPassedThroughJSONNestedNode {
nodeType: string;
nodeTypeIndicator: string;
createItemString: (data: any, collectionLimit: number) => string;
expandable: boolean;
expandable?: boolean;
}

interface State {
expanded: boolean;
}
export default function JSONNestedNode(props: Props) {
const {
getItemString,
nodeTypeIndicator,
nodeType,
data = [],
hideRoot,
createItemString,
styling,
collectionLimit,
keyPath,
labelRenderer,
expandable = true,
isCircular,
level = 0,
shouldExpandNode,
} = props;

function getStateFromProps(props: Props) {
// calculate individual node expansion if necessary
const expanded = !props.isCircular
? props.shouldExpandNode(props.keyPath, props.data, props.level)
: false;
return {
expanded,
};
}
const [expanded, setExpanded] = useState<boolean>(() => {
return !isCircular ? shouldExpandNode(keyPath, data, level) : false;
});

export default class JSONNestedNode extends React.Component<Props, State> {
static propTypes = {
getItemString: PropTypes.func.isRequired,
nodeTypeIndicator: PropTypes.any,
nodeType: PropTypes.string.isRequired,
data: PropTypes.any,
hideRoot: PropTypes.bool.isRequired,
createItemString: PropTypes.func.isRequired,
styling: PropTypes.func.isRequired,
collectionLimit: PropTypes.number,
keyPath: PropTypes.arrayOf(
PropTypes.oneOfType([PropTypes.string, PropTypes.number])
).isRequired,
labelRenderer: PropTypes.func.isRequired,
shouldExpandNode: PropTypes.func,
level: PropTypes.number.isRequired,
sortObjectKeys: PropTypes.oneOfType([PropTypes.func, PropTypes.bool]),
isCircular: PropTypes.bool,
expandable: PropTypes.bool,
};

static defaultProps = {
data: [],
circularCache: [],
level: 0,
expandable: true,
};

constructor(props: Props) {
super(props);
this.state = getStateFromProps(props);
}

UNSAFE_componentWillReceiveProps(nextProps: Props) {
const nextState = getStateFromProps(nextProps);
if (getStateFromProps(this.props).expanded !== nextState.expanded) {
this.setState(nextState);
// When certain props change, we need to re-compute whether our node should be in an expanded state
useEffect(() => {
setExpanded(() => {
return !isCircular ? shouldExpandNode(keyPath, data, level) : false;
});
Comment on lines +124 to +126
Copy link
Member

@Methuselah96 Methuselah96 Feb 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason this is passed as a function into setExpanded? This seems like it would be equivalent to just calculating it without a function.

}, [isCircular, data, keyPath, level, shouldExpandNode]);
Comment on lines +122 to +127
Copy link
Member

@Methuselah96 Methuselah96 Feb 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this would reintroduce the undesired behavior discussed here since shouldExpandNode is often intended to only indicate whether a node should be expanded on the first render. This was fixed by evaluating shouldExpandNode for the previous props and the current props and only updating the expanded state if they differ.

We're unfortunately stuck in an awkward situation where this component is partially controlled and it's unclear when shouldExpandNode should be called. We either need to preserve the previous behavior (which seems difficult if we don't want to keep around a reference to the previous props) or come up with a better solution that avoids the difficulties that arise from a prop that allows for partially controlled state. Feel free to pitch some ideas so we can discuss before putting too much working into a single solution. I'll keep thinking about it myself.


const renderedChildren =
expanded || (hideRoot && props.level === 0)
? renderChildNodes({ ...props, level: level + 1 })
: null;

const itemType = (
<span {...styling('nestedNodeItemType', expanded)}>
{nodeTypeIndicator}
</span>
);
const renderedItemString = getItemString(
nodeType,
data,
itemType,
createItemString(data, collectionLimit),
keyPath
);
const stylingArgs = [keyPath, nodeType, expanded, expandable] as const;

const expandableLatest = useRef<boolean>(expandable);
expandableLatest.current = expandable;
const handleClick = useCallback(() => {
if (expandableLatest.current) {
setExpanded((prevValue) => !prevValue);
Comment on lines +148 to +152
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the intention of the useRef is to avoid recreating handleClick and causing unnecessary re-renders if expandable changes? I'd rather avoid this if possible since it's discouraged by the React docs. My intuition is that the expandable prop is not going to change often and when it does, I think it's going to require a re-render anyway so it seems like a premature optimization at the cost of more complicated code.

}
}

shouldComponentUpdate(nextProps: Props, nextState: State) {
return (
!!Object.keys(nextProps).find(
(key) =>
key !== 'circularCache' &&
(key === 'keyPath'
? nextProps[key].join('/') !== this.props[key].join('/')
: nextProps[key as keyof Props] !== this.props[key as keyof Props])
) || nextState.expanded !== this.state.expanded
);
}

render() {
const {
getItemString,
nodeTypeIndicator,
nodeType,
data,
hideRoot,
createItemString,
styling,
collectionLimit,
keyPath,
labelRenderer,
expandable,
} = this.props;
const { expanded } = this.state;
const renderedChildren =
expanded || (hideRoot && this.props.level === 0)
? renderChildNodes({ ...this.props, level: this.props.level + 1 })
: null;

const itemType = (
<span {...styling('nestedNodeItemType', expanded)}>
{nodeTypeIndicator}
}, []);

return hideRoot ? (
<li {...styling('rootNode', ...stylingArgs)}>
<ul {...styling('rootNodeChildren', ...stylingArgs)}>
{renderedChildren}
</ul>
</li>
) : (
<li {...styling('nestedNode', ...stylingArgs)}>
{expandable && (
<JSONArrow
styling={styling}
nodeType={nodeType}
expanded={expanded}
onClick={handleClick}
/>
)}
<label
{...styling(['label', 'nestedNodeLabel'], ...stylingArgs)}
onClick={handleClick}
>
{labelRenderer(...stylingArgs)}
</label>
<span
{...styling('nestedNodeItemString', ...stylingArgs)}
onClick={handleClick}
>
{renderedItemString}
</span>
);
const renderedItemString = getItemString(
nodeType,
data,
itemType,
createItemString(data, collectionLimit),
keyPath
);
const stylingArgs = [keyPath, nodeType, expanded, expandable] as const;

return hideRoot ? (
<li {...styling('rootNode', ...stylingArgs)}>
<ul {...styling('rootNodeChildren', ...stylingArgs)}>
{renderedChildren}
</ul>
</li>
) : (
<li {...styling('nestedNode', ...stylingArgs)}>
{expandable && (
<JSONArrow
styling={styling}
nodeType={nodeType}
expanded={expanded}
onClick={this.handleClick}
/>
)}
<label
{...styling(['label', 'nestedNodeLabel'], ...stylingArgs)}
onClick={this.handleClick}
>
{labelRenderer(...stylingArgs)}
</label>
<span
{...styling('nestedNodeItemString', ...stylingArgs)}
onClick={this.handleClick}
>
{renderedItemString}
</span>
<ul {...styling('nestedNodeChildren', ...stylingArgs)}>
{renderedChildren}
</ul>
</li>
);
}

handleClick = () => {
if (this.props.expandable) {
this.setState({ expanded: !this.state.expanded });
}
};
<ul {...styling('nestedNodeChildren', ...stylingArgs)}>
{renderedChildren}
</ul>
</li>
);
}

JSONNestedNode.propTypes = {
getItemString: PropTypes.func.isRequired,
nodeTypeIndicator: PropTypes.any,
nodeType: PropTypes.string.isRequired,
data: PropTypes.any,
hideRoot: PropTypes.bool.isRequired,
createItemString: PropTypes.func.isRequired,
styling: PropTypes.func.isRequired,
collectionLimit: PropTypes.number,
keyPath: PropTypes.arrayOf(
PropTypes.oneOfType([PropTypes.string, PropTypes.number])
).isRequired,
labelRenderer: PropTypes.func.isRequired,
shouldExpandNode: PropTypes.func,
level: PropTypes.number.isRequired,
sortObjectKeys: PropTypes.oneOfType([PropTypes.func, PropTypes.bool]),
isCircular: PropTypes.bool,
expandable: PropTypes.bool,
};
Loading