Skip to content
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

Converting circular structure to JSON #133

Closed
arolson101 opened this issue Oct 27, 2017 · 23 comments
Closed

Converting circular structure to JSON #133

arolson101 opened this issue Oct 27, 2017 · 23 comments

Comments

@arolson101
Copy link

I am passing a react element as a prop to a custom component like this:

<TextField addonBefore={<div>addon</div>}/>

this results in:

Uncaught TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
    at t.value (preview.bundle.js:formatted:382618)
    at checkShouldComponentUpdate (preview.bundle.js:formatted:135775)
    at updateClassInstance (preview.bundle.js:formatted:136091)
    at updateClassComponent (preview.bundle.js:formatted:136311)
    at beginWork (preview.bundle.js:formatted:136691)
    at performUnitOfWork (preview.bundle.js:formatted:138635)
    at workLoop (preview.bundle.js:formatted:138744)
    at HTMLUnknownElement.callCallback (preview.bundle.js:formatted:127420)
    at Object.invokeGuardedCallbackDev (preview.bundle.js:formatted:127459)

in checkShouldComponentUpdate it's testing JSON.stringify(a) !== JSON.stringify(u). I'm not quite sure why the structure is circular but it would be nice if this was supported in version 2 (it worked in version 1)

@joepuzzo
Copy link
Contributor

Hmm you are not alone. Someone else is having similar problem.. i need to revisit why i added that check in shouldComponentUpdate.

@joepuzzo
Copy link
Contributor

@arolson101 Hey turns out that optimization was not needed anymore because i updated react form to use react-redux and it no longer used state. You should be all set now, get the latest version 2.1.1 and let me know it this problem is solved.

@joepuzzo
Copy link
Contributor

Hmm although removing this may cause some performance issues in IE i think i will disable it by default and add an optimization flag.

@joepuzzo
Copy link
Contributor

Ok so i removed this optimization by default but if you need it then pass in optimize prop. V2.1.2

@joepuzzo
Copy link
Contributor

Update: working on a solution that adds this optimization by default and does not have circular json issue.

@joepuzzo
Copy link
Contributor

Ok so im now taking advantage of https://www.npmjs.com/package/circular-json so dont even worry about that temp solution. Your issue should be fixed now :)

@arolson101
Copy link
Author

Thank you for your responsiveness!
In my scenario, calling stringify is not an optimization; significant time is spent in the stringify. Is there a way to disable, or better yet change it to opt-in? I'd rather see a reference or equality test and be done with it

@joepuzzo
Copy link
Contributor

Have u updated and tried with the latest? Your og issue was that u were getting circular reference and u should not be any more. I could maybe add a flag thats says “dontOptimize” but trust me this will slow the form down the second u start getting big forms with lots of rendering

@arolson101
Copy link
Author

Yes, this was on 2.1.5
The problems with "big forms with lots of rendering" sounds like redux-form, which is part of the reason why I switched to react-form.
I would suggest using a library to test either shallow (or deep, if necessary) equality rather than stringifying.

@joepuzzo
Copy link
Contributor

Stringifying is the fastest way to deep compare in JS

@tannerlinsley
Copy link
Collaborator

@joepuzzo, it seems like there must be a better way to handle these change detections. I would really love it if there was no stringification at all. Thoughts?

@tannerlinsley
Copy link
Collaborator

I would simply loop over the keys of the old and new props, and detecting strict equality differences. As soon as you find a difference, you should bail out of the checking, so no time is wasted. I'll help you write this if you want.

@joepuzzo
Copy link
Contributor

joepuzzo commented Oct 30, 2017

Hmm i have done reading on this before and it seemed like conclusion was json.stringify is the fastest most efficient way to do comparisons on complex objects.. is this wrong ?

@arolson101
Copy link
Author

Here is a good summary of the stringify equality: http://www.mattzeunert.com/2016/01/28/javascript-deep-equal.html

What a surprise! The reason I was using deepEqual was that I thought jsonEqual would be an order of magnitude slower. Turns out it’s actually faster!

Caveats to the performance comparison
However, on the whole it’s still much better to use deepEqual. (It’s only 42% slower anyway.)

There are two cases where deepEqual is faster than a JSON comparison.

if the two objects are referentially equal jsonEqual will still generate the two complete JSON strings. deepEqual on the other hand will immediately see that the two options are the same and finish 8000 times more quickly.

While jsonEqual is faster at confirming that two objects are equal, deepEqual is much faster at finding out that they aren’t. As soon as it finds two properties that don’t match up it returns false, rather than continuing to look for differences.

@arolson101
Copy link
Author

Is there a reason for doing a deep equality rather than a shallow one?
Also, since you're dealing with props, users of your library might decide to pass down a large object - e.g. part a redux store or (in my case) a react element

@joepuzzo
Copy link
Contributor

So this is a difficult problem to solve lol. React-form uses reacts context to give all FormFields access to the form api ( this is one of the most important parts of this lib ). Well this is very powerfull, it comes with the shitty side affect that you see in redux-form. Every single field will re-render. So in order to prevent this, the FormField takes advantage of reacts shouldComponentUpdate lifecycle method. This solves one problem but causes another, if someone changed any other prop passed to a form field, it would not cause a rerender. So, in order to rerender if any other prop was changed ( just like react noramlly works ) I needed to compare those props.

@joepuzzo
Copy link
Contributor

Ok i have done more reading and educated myself more lol.. i think the solution is what you posted @arolson101 http://www.mattzeunert.com/2016/01/28/javascript-deep-equal.html.

@joepuzzo
Copy link
Contributor

joepuzzo commented Oct 31, 2017

@tannerlinsley it looks like the deepEqual function does exactly what you wanted to write While jsonEqual is faster at confirming that two objects are equal, deepEqual is much faster at finding out that they aren’t. As soon as it finds two properties that don’t match up it returns false, rather than continuing to look for differences.

@joepuzzo joepuzzo reopened this Oct 31, 2017
@tannerlinsley
Copy link
Collaborator

While deepEqual would work, that would effectively be doing more than what is normally needed in a standard react component. 99.9% of the time, react optimization is done at the prop level, or in other words, the keys of the props object. So in the case of #134, it's plenty sufficient to just compare the old and new props of the child element.

@tannerlinsley
Copy link
Collaborator

See #136

@joepuzzo
Copy link
Contributor

Checked out PRs and added some comments. We are close to resolution once those PRs are good to go.

@tannerlinsley
Copy link
Collaborator

This should be fixed now that we're doing a shallow equality comparison :)

@arolson101
Copy link
Author

2.2.0 is much better, thank you!

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

No branches or pull requests

3 participants