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

Circular json can possibly hang. #778

Closed
shilman opened this issue Apr 15, 2017 · 13 comments
Closed

Circular json can possibly hang. #778

shilman opened this issue Apr 15, 2017 · 13 comments

Comments

@shilman
Copy link
Member

shilman commented Apr 15, 2017

Issue by rhalff
Wednesday Nov 02, 2016 at 02:55 GMT
Originally opened as storybook-eol/storybook-addon-actions#26


It can be replicated by replacing circular with something like window within:

https://github.com/kadirahq/storybook-addon-actions/blob/master/.storybook/stories.js#L20

The click on the circular button will cause the action to never be logged.

I've tried replacing json-stringify-safe with circular-json which seems to behave better.
But even then after the 4th click circular-json will throw a circular reference error.

I now know how to circumvent this by just not logging something with circular references, however when this bug is encountered it's not immediately clear the hang originates from within storybook which makes one try to debug the application code itself first.

I think replacing json-stringify-safe with circular-json would already be an improvement.

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by leMaik
Wednesday Nov 02, 2016 at 18:10 GMT


Also facing this problem... Wasted hours just to find out that storybook caused this. 😢

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by jonnyelliot
Thursday Feb 16, 2017 at 14:33 GMT


Also hit this issue.

@shilman
Copy link
Member Author

shilman commented Apr 15, 2017

Comment by ndelangen
Wednesday Apr 05, 2017 at 21:06 GMT


A PR fixing this would be super welcome! Would you care to give it a go @rhalff ?

@ndelangen
Copy link
Member

@rhalff @leMaik @jonnyelliot Any of you feel like contributing to help resolve this issue? ♥️

@leMaik
Copy link

leMaik commented Jul 7, 2017

@ndelangen I'd like to, but I have no idea how to fix this. 😕

@jonnyelliot
Copy link

@ndelangen I can take a look at this.

@ndelangen
Copy link
Member

@leMaik Here's why i thought it would be pretty simple:

I think replacing json-stringify-safe with circular-json would already be an improvement.

Maybe you and @jonnyelliot can contact each other and resolve it together. If you need my help, just ask! 🥇

@ndelangen
Copy link
Member

@jonnyelliot Is this still something you're interested in to fix?

Did you get stuck somewhere? maybe I can help!

@dangreenisrael
Copy link
Member

Hey guys, is someone actively working on this? If not I can take a stab at it this weekend.

@rhalff
Copy link
Contributor

rhalff commented Sep 20, 2017

I could have a look, no guarantees though

Btw, the link in the original post should become:
https://github.com/storybooks/storybook/blob/master/addons/actions/.storybook/stories.js#L20

The current format function only test for a 'SyntheticEvent' but there are many more cases where recursion will fail:
https://github.com/storybook-eol/storybook-addon-actions/blob/master/src/preview.js#L7

Node's build-in util.inspect could be used, storybook action stringifies to log something to the log window so precise recursion is not needed.

@ndelangen
Copy link
Member

@dangreenisrael @rhalff a PR to fix this proper would be fantastic!

@goulashify
Copy link

goulashify commented Oct 5, 2017

Nice catch people!

So i've hit this and (just like you) spent about an hour on this, to my (and may your!) salvation, moving the event to it's own argument will solve this:

before:

const MyAwesomeComponent = ({ onClick }) => (
  <button 
    onClick={(event) => onClick({ event, awesome: true })} 
  />
);

after:

const MyAwesomeComponent = ({ onClick }) => (
  <button 
    onClick={(event) => onClick(event, { awesome: true })} 
  />
);

@ndelangen
Copy link
Member

I think this is fixed right?

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

No branches or pull requests

7 participants