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

Revert #20 causing regression and add tests for reviver #26

Merged
merged 10 commits into from
Dec 1, 2018
Merged

Revert #20 causing regression and add tests for reviver #26

merged 10 commits into from
Dec 1, 2018

Conversation

zalmoxisus
Copy link
Collaborator

@zalmoxisus zalmoxisus commented Dec 1, 2018

As discussed in #24, it was introduced a regression in #20, so I think it's better to revert it. Additionally to handling references (as per #19), it's disabling stringifying of all special types, also decreases the performance adding another stringifying and because not using needsRetrocycle it will also add extra operations during parsing when not needed.

I'll look into #19 later, that could be already implemented in #7.

@zalmoxisus zalmoxisus changed the title Revert #20 causing regression and add test for reviver Revert #20 causing regression and add tests for reviver Dec 1, 2018
@zalmoxisus
Copy link
Collaborator Author

zalmoxisus commented Dec 1, 2018

Looks like #7 is only removing circular references, but still adding references otherwise:

var obj1 = { a: 1 };
var obj = { "prop1": obj1, "prop2": { "prop3": obj1 } };
jsan.stringify(obj, null, null, {circular: '∞'});

It's returning

'{"prop1":{"a":1},"prop2":{"prop3":{"$jsan":"$.prop1"}}}'

So we'd need an additional option to disable adding references, but not sure if it will hang for circular structures in that case.

@zalmoxisus
Copy link
Collaborator Author

zalmoxisus commented Dec 1, 2018

Merging this and adding refs option in 8021553.

@zalmoxisus zalmoxisus merged commit 13a206a into kolodny:master Dec 1, 2018
@kolodny
Copy link
Owner

kolodny commented Dec 2, 2018

Thanks for looking into and fixing this

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

Successfully merging this pull request may close these issues.

2 participants