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

use stable stringify for better objects diff #573

Merged
merged 2 commits into from May 2, 2016
Merged

Conversation

@iamdoron
Copy link
Contributor

iamdoron commented May 2, 2016

afterstable

instead of:
beforestable

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented May 2, 2016

This won't work with a circular object.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented May 2, 2016

what do you mean by 'this won't work'? It should act the same as before (all tests have passed)

@Marsup

This comment has been minimized.

Copy link
Member

Marsup commented May 2, 2016

My bad, I thought json-stringify-safe was more clever than it really is.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented May 2, 2016

Yes, it just outputs "[Circular]".

@@ -13,6 +14,12 @@ const internals = {
width: 50
};

internals.stringify = (obj, serializer, indent, decycler) => {

return StableStringify(

This comment has been minimized.

Copy link
@Marsup

Marsup May 2, 2016

Member

Can't we just do StableStringify(obj, { space: indent, replacer: StringifySafe.getSerialize(serializer) }) ?

This comment has been minimized.

Copy link
@iamdoron

iamdoron May 2, 2016

Author Contributor

yes, we can 👍
I changed it

@arb

This comment has been minimized.

Copy link
Contributor

arb commented May 2, 2016

I would maybe recommend "fast-safe-stringify" as an alternative since I think it's the fastest one out that that doesn't crash on circular references.

@iamdoron

This comment has been minimized.

Copy link
Contributor Author

iamdoron commented May 2, 2016

@arb - using @Marsup suggestion, it is better to use StringifySafe.getSerialize(), as it can be composed with StableStringify seamlessly. There's no such interface in fast-safe-stringify

@geek geek added the feature label May 2, 2016
@geek geek added this to the 10.4.0 milestone May 2, 2016
@geek geek self-assigned this May 2, 2016
@geek geek merged commit 71c2187 into hapijs:master May 2, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@geek

This comment has been minimized.

Copy link
Member

geek commented May 2, 2016

@iamdoron well done, this will be a nice improvement for our console reporter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.