Skip to content

Update unsafe_componentWillReceiveProps react-json-tree #644

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.

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

Conversation

BrunoFenzl
Copy link

@BrunoFenzl BrunoFenzl commented Sep 25, 2020

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.

@Methuselah96
Copy link
Member

Methuselah96 commented Sep 28, 2020

@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.

@BrunoFenzl
Copy link
Author

@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.

@Methuselah96
Copy link
Member

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

@yesh0907
Copy link

Hi, can this PR be merged? Thanks.

@Methuselah96
Copy link
Member

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.

@defunctzombie
Copy link

@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?

@Methuselah96
Copy link
Member

@defunctzombie Yeah, go for it!

@Methuselah96
Copy link
Member

Resolved by #1288.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants