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 #1881

Merged
merged 12 commits into from
Sep 22, 2017
Merged

Circular json can possibly hang #1881

merged 12 commits into from
Sep 22, 2017

Conversation

rhalff
Copy link
Contributor

@rhalff rhalff commented Sep 22, 2017

  • replaced json-stringify-safe with a custom version of JSON-js's decycle and retrocycle.
  • hide non-enumerable properties:
    It makes no sense to render these while the logged objects come directly from JSON.parse.
    The non-enumerability is used to set some properties to restore the original constructor names. react-inspector will then use these to render the correct labels.
  • custom node renderer to restore the constructor names
  • if an object is marked as cyclic (by decycle) no attempt will be made to test for deep equality.
  • added a max depth of 15, when this level is reached traversal stops and three extra dots are added to the label: [<name>...]
  • sort object keys

Perhaps the documentation should make clear that whatever is logged is not the original object.
This is due to the objects being stringified first and then restored again.

fixes #778

@codecov
Copy link

codecov bot commented Sep 22, 2017

Codecov Report

Merging #1881 into release/3.3 will decrease coverage by 0.07%.
The diff coverage is 22.01%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #1881      +/-   ##
===============================================
- Coverage        22.25%   22.18%   -0.08%     
===============================================
  Files              324      326       +2     
  Lines             6339     6465     +126     
  Branches           797      812      +15     
===============================================
+ Hits              1411     1434      +23     
- Misses            4333     4448     +115     
+ Partials           595      583      -12
Impacted Files Coverage Δ
...ddons/actions/src/components/ActionLogger/index.js 33.33% <ø> (ø) ⬆️
...ddons/actions/src/containers/ActionLogger/index.js 10% <0%> (-0.53%) ⬇️
addons/actions/src/preview.js 61.53% <100%> (+11.53%) ⬆️
...ctions/src/components/ActionLogger/NodeRenderer.js 9.52% <11.11%> (ø)
addons/actions/src/util.js 19.62% <24.13%> (ø)
addons/info/src/components/types/InstanceOf.js 16.66% <0%> (ø) ⬆️
addons/info/src/components/types/OneOf.js 14.28% <0%> (ø) ⬆️
addons/info/src/components/PropVal.js 41.41% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/filters.js 47.36% <0%> (ø) ⬆️
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fd1704...b3fac7e. Read the comment docs.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -23,5 +23,8 @@ describe('preview', () => {
expect(channel.emit.mock.calls[0][1].id).toBe('42');
expect(channel.emit.mock.calls[1][1].id).toBe('24');
});
it('should be able to handle cyclic object without hanging', () => {
action('foo')(process);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha, that's kind of funny actually!

@ndelangen ndelangen changed the base branch from master to release/3.3 September 22, 2017 05:53
@@ -0,0 +1,117 @@
export const CLASS_NAME_KEY = '$___storybook.className';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might this be it's own npm module maybe?

@ndelangen
Copy link
Member

Thank you for this PR @rhalff ! 🙇

I have a question about if we could extract the util you added into a separate npm module, maybe now, or later. But I won't block merging for that.

@ndelangen ndelangen requested a review from a team September 22, 2017 08:05
@ndelangen
Copy link
Member

Waiting for a second review

@danielduan
Copy link
Member

this might fix - #1839

Copy link
Member

@danielduan danielduan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM +1

this feels like a patch version to me. thoughts @ndelangen @shilman?

@ndelangen ndelangen merged commit eefae70 into storybookjs:release/3.3 Sep 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants