kuy / redux-tooltip

A tooltip React component for Redux.
130 stars 31 forks source link

Origin contains CIRCULAR attributes in state #45

Open p0o opened 7 years ago

p0o commented 7 years ago

I had this weird circular error when I setup a basic implementation of this lib on my codebase:

Uncaught TypeError: Converting circular structure to JSON

Basically circular structure means an object which is containing elements that refer to the same object and these type of objects are not serializable. So after using an Origin for any element I had this error because somewhere in my react setup I had a serialization like this:

JSON.stringify(state)

In my case it leads to crashing redux dev tools and redux dev tools also show the circulation inside state.tooltip[NAME].Origin._hostnode key as below:

screen shot 2017-01-08 at 5 35 09 pm

Any ideas?

kuy commented 7 years ago

@p0o I could also find _hostNode: [CIRCULAR] in simple example demo. But I think it's difficult to avoid Uncaught TypeError: Converting circular structure to JSON error by fixing redux-tooltip because the circulated structure is built by React. This library just stores it in the state as origin.

Solution 1: Use Redux DevTools Extension instead of Redux DevTools. This extension resolves the issue you're facing.

Solution 2: Wrap JSON.stringify with try-catch block. You can define your own stringify function with try-catch. For example, Redux DevTools Extension catches an exception and shows [CIRCULAR] in the inspector.

redux-devtools-extension/src/app/api/index.js

function tryCatchStringify(obj) {
  try {
    return JSON.stringify(obj);
  } catch (err) {
    /* eslint-disable no-console */
    if (process.env.NODE_ENV !== 'production') console.log('Failed to stringify', err);
    /* eslint-enable no-console */
    return jsan.stringify(obj, null, null, { circular: '[CIRCULAR]' });
  }
}
screen shot 0029-01-08 at 23 42 21
zalmoxisus commented 7 years ago

Vanilla Redux DevTools uses JSON.stringify for persisting the state in persistState enhancer and the extension uses it as well when you add debug_session in the url. It was planned to be fixed in https://github.com/gaearon/redux-devtools/pull/271. However, we'll remove this enhancer from Redux DevTools Extension 3.0 in favour of https://github.com/zalmoxisus/redux-devtools-extension/pull/276, which I recommend to use to persist data.

However, you might want to get your data with circular references back when persisting the state. In this case you should add serializeState parameter as true and you'll have the data instead of [CIRCULAR], though there could be some issues.

kuy commented 7 years ago

@zalmoxisus Thanks for helping! Redux DevTools Extension v3 looks smart.

@p0o I assumed you just want to see the state by using JSON.stringify in somewhere of your code. But if you need to store the state, please follow the advice by @zalmoxisus.

p0o commented 7 years ago

Thank you both for answers.

Right now I'm not persisting the tooltip part of my state so there shouldn't be a problem. The thing is I am not sure if this approach is correct and does storing circular data in state creates any performance problems or not? If you think it's common and we wouldn't have deeply nested circular references that makes the serialization slow, I would stick to the custom JSON.stringfiy approach for now.

zalmoxisus commented 7 years ago

I guess storing a reference to the React component will not allow to persist the data (I'm referring not only to the Redux DevTools, but also for cases when persisting the state in localStorage or on the server side). Even if there are no circular data the reference will be lost.

However, usually we don't need to persist a tooltip. If so, you could easily remove the component data for JSON.stringify, by adding a custom toJSON method for actions and states, like we do for events here.

p0o commented 7 years ago

@zalmoxisus this toJSON method looks very neat. Should I handle its execution by myself manually or there is a better way to do so? I believe you're doing it in a specific way in Redux DevTools?

zalmoxisus commented 7 years ago

@p0o I didn't look into the sources, but I guess those actions and states come from redux-tooltip and that should be included here, unless you reimplement its action creators and reducers.

zalmoxisus commented 7 years ago

To clarify, toJSON method of an object is used natively by JSON.stringify. For example, it's how ImmutableJS is specifying how its custom data types will be stringified.

p0o commented 7 years ago

Awesome @zalmoxisus that would solve my problem 100%.

Mentioning that, @kuy I think it would be a good idea to add this method to actions and reducers of redux-tooltip so no one else faces this error in the future.

kuy commented 7 years ago

I didn't know overwriting JSON.stringify by toJSON. Awesome tips! Thanks @zalmoxisus.

@p0o Yeah, I'd like to add toJSON function to our actions only if NODE_ENV=development by default. I assume most people don't have interest in the inside of origin property. For someone who interests in it, I also provide an option to see the detail of origin. Thanks for your proposal.