Remove UNSAFE_componentWillReceiveProps from react-json-tree #1049

defunctzombie · 2022-01-23T22:37:12Z

Reworking of #644 to remove UNSAFE_componentWillReceiveProps which is deprecated in react 17 and likely removed in 18.

Components that used UNSAFE_componentWillReceiveProps are converted to functional components with hooks.

Update JSONTree to a functional component to remove the unsave receive props. The functional component uses a useMemo hook on the theme to avoid creating the theme style unless the theme has changed.

packages/react-json-tree/src/JSONNestedNode.tsx

defunctzombie · 2022-01-24T20:03:48Z

packages/react-json-tree/src/index.tsx

@@ -47,133 +46,56 @@ const defaultLabelRenderer = ([label]: (string | number)[]) => (
 );
 const noCustomNode = () => false;

-function checkLegacyTheming(theme: Theme | undefined, props: Props) {


@Methuselah96 As part of this update I removed support for the deprecated styling methods. Removing these is not strictly required to do in this change (just something that made the code easier to reason about). Since this could be seen as a breaking change I am happy to put this back if desired.

Methuselah96 · 2022-01-29T14:47:38Z

@defunctzombie Looks like there are some type errors:

Error: src/JSONArrayNode.tsx(21,4): error TS2322: Type '{ data: any; nodeType: string; nodeTypeIndicator: string; createItemString: (data: any) => string; expandable: boolean; styling: StylingFunction; keyPath: (string | number)[]; ... 12 more ...; children?: ReactNode; }' is not assignable to type 'Pick<Props, "nodeType" | "styling" | "keyPath" | "labelRenderer" | "valueRenderer" | "circularCache" | "level" | "isCircular" | "shouldExpandNode" | "hideRoot" | "getItemString" | ... 6 more ... | "expandable">'.
  Types of property 'circularCache' are incompatible.
    Type 'any[] | undefined' is not assignable to type 'any[]'.
      Type 'undefined' is not assignable to type 'any[]'.
Error: src/JSONIterableNode.tsx(33,6): error TS2741: Property 'expandable' is missing in type '{ nodeType: string; nodeTypeIndicator: string; createItemString: (data: any, limit: number) => string; data: any; styling: StylingFunction; keyPath: (string | number)[]; ... 12 more ...; children?: ReactNode; }' but required in type 'Pick<Props, "nodeType" | "styling" | "keyPath" | "labelRenderer" | "valueRenderer" | "circularCache" | "level" | "isCircular" | "shouldExpandNode" | "hideRoot" | "getItemString" | ... 6 more ... | "expandable">'.
Error: src/JSONObjectNode.tsx(20,4): error TS2322: Type '{ data: any; nodeType: string; nodeTypeIndicator: string; createItemString: (data: any) => string; expandable: boolean; styling: StylingFunction; keyPath: (string | number)[]; ... 12 more ...; children?: ReactNode; }' is not assignable to type 'Pick<Props, "nodeType" | "styling" | "keyPath" | "labelRenderer" | "valueRenderer" | "circularCache" | "level" | "isCircular" | "shouldExpandNode" | "hideRoot" | "getItemString" | ... 6 more ... | "expandable">'.
  Types of property 'circularCache' are incompatible.
    Type 'any[] | undefined' is not assignable to type 'any[]'.
      Type 'undefined' is not assignable to type 'any[]'.

changeset-bot · 2022-01-30T22:14:50Z

⚠️ No Changeset found

Latest commit: cf66127

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

defunctzombie · 2022-01-30T22:29:24Z

@Methuselah96 I looked at the types here and can't make sense of where this error comes from or what the intended behavior is. I did not change any of the Prop types in this PR.

The error comes from a miss-match between these two types:

https://github.com/reduxjs/redux-devtools/blob/main/packages/react-json-tree/src/types.ts#L54
https://github.com/reduxjs/redux-devtools/blob/main/packages/react-json-tree/src/types.ts#L65

One being optional for circularCache.

Methuselah96 · 2022-01-30T22:36:51Z

Previously, circularCache was defaulted to an empty array in JSONNestedNode if it wasn't provided, however it looks like this PR does not initialize the circular cache (and perhaps also fails to update it?).

defunctzombie · 2022-01-30T23:58:44Z

Even if I updated JSONNestedNode to set a default for circularCache, that doesn't prevent the error. The issue is that JSONArrayNode props are of type CircularPropsPassedThroughJSONNode which extend JSONNestedNodeCircularPropsPassedThroughJSONNode which has circularCache?: any[]; however JSONNestedNode props are CircularPropsPassedThroughJSONNestedNode which extend JSONNestedNodeCircularPropsPassedThroughJSONNestedNode which has circularCache: any[];.

So when JSONArrayNode does <JSONNestedNode {...props} ...etc circularCache types don't match since JSONNestedNode doesn't allow undefined for circularCache. Its possible that the defaultProps from before is treated as turning circularCache: any[] into circularCache?: any[] even tho the Props type does not allow circularCache to be undefined.

The two fixes are:

add circularCache={props.circularCache ?? emptyArray} to JSONArrayNode's use of JSONNestedNode
update the Props for JSONNestedNode to allow circularCache to be optional

Preferences?

Methuselah96 · 2022-01-31T00:46:01Z

Right, if a prop is specified in defaultProps then TypeScript will make that prop optional when the component is used.

Option 1 is not preferable because JSONArrayNode is not the only component that uses JSONNestedNode (JSONIterableNode and JSONObjectNode do as well). Option 2 sounds good to me (accompanied with actually defaulting circularCache to an empty array and updating it appropriately).

…le_ props for JSONNestedNode

defunctzombie · 2022-01-31T05:17:04Z

I've updated the typescript prop types to reflect allowing undefined.

defunctzombie · 2022-02-08T16:32:47Z

@Methuselah96 anything else that I need to do on this PR before it can be merged?

Methuselah96

Thanks for taking this on! I think there are a few things that still need to be addressed before this can be merged. Also, can you add a changeset that includes a description of the change and notes the breaking change of removing the legacy theming props?

Methuselah96 · 2022-02-13T19:15:58Z

packages/react-json-tree/src/index.tsx

+      <JSONNode
+        postprocessValue={postprocessValue}
+        hideRoot={hideRoot}
+        styling={styling}
+        shouldExpandNode={shouldExpandNode}
+        getItemString={getItemString}
+        labelRenderer={labelRenderer}
+        valueRenderer={valueRenderer}
+        isCustomNode={isCustomNode}
+        collectionLimit={collectionLimit}
+        keyPath={hideRoot ? [] : keyPath}
+        value={postprocessValue(value)}
+      />


Are we failing to pass-through sortObjectKeys here? Speaking of which, can we make it match the pattern of the other props by making sortObjectKeys required in JSONNestedNodeCircularPropsPassedThroughJSONTree, default it to false in this component, and remove | undefined when it's listed in the parameters for getCollectionEntries and getEntries?

Methuselah96 · 2022-02-13T19:25:20Z

packages/react-json-tree/src/types.ts

 export type CircularPropsPassedThroughJSONNestedNode =
  SharedCircularPropsProvidedByJSONTree &
    JSONValueNodeCircularPropsPassedThroughJSONTree &
-    JSONNestedNodeCircularPropsPassedThroughJSONNestedNode;
+    JSONNestedNodeCircularPropsPassedThroughJSONNode;

 export type CircularPropsPassedThroughRenderChildNodes =
  SharedCircularPropsProvidedByJSONTree &
    JSONValueNodeCircularPropsPassedThroughJSONTree &
-    JSONNestedNodeCircularPropsPassedThroughJSONNestedNode;
+    JSONNestedNodeCircularPropsPassedThroughJSONNode;

 export type CircularPropsPassedThroughItemRange =
  SharedCircularPropsProvidedByJSONTree &
    JSONValueNodeCircularPropsPassedThroughJSONTree &
-    JSONNestedNodeCircularPropsPassedThroughJSONNestedNode;
+    JSONNestedNodeCircularPropsPassedThroughJSONNode;


Can we go ahead and get rid of these three types since they're now identical to CircularPropsPassedThroughJSONNode and just use that everywhere?

Methuselah96 · 2022-02-13T19:52:02Z

packages/react-json-tree/src/JSONNestedNode.tsx

+  const expandableLatest = useRef<boolean>(expandable);
+  expandableLatest.current = expandable;
+  const handleClick = useCallback(() => {
+    if (expandableLatest.current) {
+      setExpanded((prevValue) => !prevValue);


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.

Methuselah96 · 2022-02-13T19:54:31Z

packages/react-json-tree/src/JSONNestedNode.tsx

+    setExpanded(() => {
+      return !isCircular ? shouldExpandNode(keyPath, data, level) : false;
+    });


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.

Methuselah96 · 2022-02-13T20:26:51Z

packages/react-json-tree/src/JSONNestedNode.tsx

+  // 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;
+    });
+  }, [isCircular, data, keyPath, level, shouldExpandNode]);


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.

unematiii · 2022-06-20T18:01:59Z

@Methuselah96 @defunctzombie Is any of you still working on it?

Methuselah96 · 2022-06-20T18:11:44Z

I am not currently working on it.

defunctzombie · 2022-06-21T02:59:11Z

I too am not currently working on this.

BENcorry · 2023-01-04T03:01:39Z

i wanna to ask which version has fixed this problem

Methuselah96 · 2023-01-04T12:22:04Z

It has not been resolved yet, but I am working on a fix that I hope will be released within the next week.

Remove UNSAFE_componentWillReceiveProps from JSONTree

f70f183

Update JSONTree to a functional component to remove the unsave receive props. The functional component uses a useMemo hook on the theme to avoid creating the theme style unless the theme has changed.

defunctzombie marked this pull request as draft January 23, 2022 22:37

defunctzombie commented Jan 23, 2022

View reviewed changes

packages/react-json-tree/src/JSONNestedNode.tsx Outdated Show resolved Hide resolved

Remove UNSAFE_componentWillReceiveProps from JSONNestedNode

68dba97

defunctzombie force-pushed the roman/react-json-tree-remove-unsafe branch from c6db35f to 68dba97 Compare January 23, 2022 22:40

defunctzombie and others added 2 commits January 24, 2022 12:01

default export

9a8a6e9

Merge branch 'main' into roman/react-json-tree-remove-unsafe

6b4da1b

defunctzombie commented Jan 24, 2022

View reviewed changes

defunctzombie marked this pull request as ready for review January 24, 2022 20:04

Merge branch 'main' into roman/react-json-tree-remove-unsafe

9c00bdd

defunctzombie and others added 3 commits January 30, 2022 18:46

update prop types to allow for optional _circularCache_ and _expandab…

ed9cb23

…le_ props for JSONNestedNode

Merge branch 'main' into roman/react-json-tree-remove-unsafe

03a8af7

fix root props

ffcbe5d

defunctzombie and others added 2 commits January 31, 2022 07:28

set circularCache default in renderChildNodes

043682d

Merge branch 'main' into roman/react-json-tree-remove-unsafe

cf66127

Methuselah96 requested changes Feb 13, 2022

View reviewed changes

defunctzombie closed this Dec 11, 2022

Uh oh!

Remove UNSAFE_componentWillReceiveProps from react-json-tree #1049

Remove UNSAFE_componentWillReceiveProps from react-json-tree #1049

Uh oh!

Conversation

defunctzombie commented Jan 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

defunctzombie Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

Methuselah96 commented Jan 29, 2022

Uh oh!

changeset-bot bot commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

defunctzombie commented Jan 30, 2022

Uh oh!

Methuselah96 commented Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

defunctzombie commented Jan 30, 2022

Uh oh!

Methuselah96 commented Jan 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

defunctzombie commented Jan 31, 2022

Uh oh!

defunctzombie commented Feb 8, 2022

Uh oh!

Methuselah96 left a comment

Choose a reason for hiding this comment

Uh oh!

Methuselah96 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

Methuselah96 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

Methuselah96 Feb 13, 2022

Choose a reason for hiding this comment

Uh oh!

Methuselah96 Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Methuselah96 Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unematiii commented Jun 20, 2022

Uh oh!

Methuselah96 commented Jun 20, 2022

Uh oh!

defunctzombie commented Jun 21, 2022

Uh oh!

BENcorry commented Jan 4, 2023

Uh oh!

Methuselah96 commented Jan 4, 2023

Uh oh!

Uh oh!

defunctzombie commented Jan 23, 2022 •

edited

Loading

changeset-bot bot commented Jan 30, 2022 •

edited

Loading

Methuselah96 commented Jan 30, 2022 •

edited

Loading

Methuselah96 commented Jan 31, 2022 •

edited

Loading

Methuselah96 Feb 13, 2022 •

edited

Loading

Methuselah96 Feb 13, 2022 •

edited

Loading