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

Jsoncycles #5185

Closed
wants to merge 0 commits into from
Closed

Jsoncycles #5185

wants to merge 0 commits into from

Conversation

ariel-bentu
Copy link

fixes #5121 - circular references in JSON in RPC traffic

It uses modified code (converted to typescript) from https://raw.githubusercontent.com/douglascrockford/JSON-js/master/cycle.js

It detects circular references while stringifying JSON, and converts to to a text reference. later after parse, it can restore the right reference.

This was important to support https://github.com/mtxr/vscode-sqltools run in Theia.

@vince-fugnitto
Copy link
Member

@ariel-bentu unfortunately, there are a few problems with the PR before we can continue with a review.

It looks like during the amend, the PR got a little messed up, can you make sure that the PR only has one commit (your original commit).

Also, since you copied code, we will first need to make sure the original license is compatible with our project, and that we register a CQ for the copied code.

There are also build errors picked up by the CI, can you resolve them?

@ariel-bentu
Copy link
Author

No problem, it is still not ready apparently. But as I make it ready, I would like feedback on the idea, to use douglas crockford's cycle.js code, which remove the cycle, but also can recreate them after json.parse. it comes with some price - it clones most of the object before stringify, and also maintains a WeakMap of all objects and arrays in the strigify-ed object while removing the cycles - which has some transient memory impact. If we wish to seamlessly support cycled object - I guess this is the way. So if you feel this is right - I will proceed with making it complete.
Thanks

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