redux-devtools icon indicating copy to clipboard operation
redux-devtools copied to clipboard

Published 20 hours ago • verified publisher icon reduxjs

Update unsafe_componentWillReceiveProps react-json-tree

Open BrunoFenzl opened this issue 3 years ago • 7 comments

This PR attempts to solve #635. unsafe_componentWillReceiveProps is currently used in two components: JSONTree and JSONNestedNode.

In JSONTree I extracted the properties theme and invertTheme from Props to a separate interface and extended the State and Props interfaces with it. The reason being, that the new static method getDerivedStateFromProps does not have access to the current props, so properties that need to be compared should be mirrored in the local state object.

In JSONNestedNode there is no need to check for derived state at all. The property expanded can be copied to state and re-rendering is checked anyway in the shouldComponentUpdate lifecycle hook.

I'm looking forward to feedback and will be glad to refactor if needed.

BrunoFenzl avatar Sep 25 '20 08:09 BrunoFenzl

@BrunoFenzl Thanks for making a PR!

  1. JSONNestedNode: getStateFromProps is now only ever called in the constructor. Before this PR it was called on every render. I think you might have to use getDerivedStateFromProps in order to match the previous behavior unless I'm missing something. In the future I would like to refactor it to remove expanded from state altogether, but I don't think that's possible without calling shouldExpandNode twice since shouldComponentUpdate depends on it (which would be a breaking change).
  2. JSONTree: The React team recommends not copying props to state just to memoize a value. I would prefer using memoize-one as suggested in this blog post.

Methuselah96 avatar Sep 28 '20 23:09 Methuselah96

@Methuselah96 Thanks for your feedback!

  1. You're right. I had getDerivedStateFromProps initially in place but thought it would be redundant because of the check in shouldComponentUpdate. I see it now I was clearly wrong.
  2. I am aware of the article but didn't want to add another dependency. If it is ok for you, I'll refactor to use memoize-one.

BrunoFenzl avatar Sep 29 '20 19:09 BrunoFenzl

@BrunoFenzl Yeah, I'm fine with adding memoize-one as a dependency. Thanks!

Methuselah96 avatar Sep 29 '20 20:09 Methuselah96

Hi, can this PR be merged? Thanks.

yesh0907 avatar Aug 10 '21 18:08 yesh0907

The feedback I gave still needs to be addressed. Feel free to create your own PR since it seems like this PR might be abandoned.

Methuselah96 avatar Aug 10 '21 18:08 Methuselah96

@Methuselah96 Would you be open to a PR which removes these unsafe calls by converting the JSONTree and JSONNestedNode to function components and using hooks?

defunctzombie avatar Jan 23 '22 15:01 defunctzombie

@defunctzombie Yeah, go for it!

Methuselah96 avatar Jan 23 '22 17:01 Methuselah96

Resolved by https://github.com/reduxjs/redux-devtools/pull/1288.

Methuselah96 avatar Jan 08 '23 19:01 Methuselah96