handle cycles in json.stringify

[ariel-bentu]

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 - according to header in this file - this is the license:

    cycle.js
    2018-05-15

    Public Domain.

    NO WARRANTY EXPRESSED OR IMPLIED. USE AT YOUR OWN RISK.

    This code should be minified before deployment.
    See http://javascript.crockford.com/jsmin.html

    USE YOUR OWN COPY. IT IS EXTREMELY UNWISE TO LOAD CODE FROM SERVERS YOU DO
    NOT CONTROL.

It detects circular references while stringify-ing 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.

[svenefftinge]

Could we use a library (i.e. https://www.npmjs.com/package/json-cycle) instead of copying code?

[ariel-bentu]

It has exactly the same code, copied... same license inside the cycle.js file. In addition, since in Theia's code we first stringify and then concat the results to a larger json, I had to extend a bit to support a baseJsonPath.

for example:

return `{${prefix}"type":${MessageType.Request},"id":"${req}","proxyId":"${rpcId}","method":"${method}",
                 "args":${JSON.stringify(JSONDecycle(args, '[\"args\"]'), ObjectsTransferrer.replacer)}}`;

also, after finishing this I realized that the issue is important in Theia due to how the tree-view is implemented, while in vscode they do not do this, because they avoided the problem from the first place (at least for tree-view) - see my last comment #5121

[ariel-bentu]

The way the tree view was implemented (TreeViewsExt, TreeViewItem, tree-view-main.tsx etc) is that the server-side object representing a TreeItem is moved (via json-rpc) from server to client upon getChildren call (specifically the original object is placed under metadata defined in TreeViewItem.
Then, upon context-menu, or inline-action-icons the relevant command is triggered from the browser via rpc passing back the object stored in the member metadata, to the command.

This back and forth move of the actual object of the tree-item is the source of some major problems:

these objects tend to have circular-references, thus pose a challenge to serialize them (fixed in my PR)
These objects could be large, and serve no purpose on the client other than to be sent back in case of menu/action-icon clicked
When they are deserialized back to object on the server they loose all the functions and private data they had before sent to the browser - thus break a lot of existing vscode-extensions.
The right way to do it (and I guess vscode do exactly that), is to avoid sending these objects to the browser. Upon menu or click on action icon, send to the server a call indicating that the menu was triggered, or the action-icon was clicked. the actual for the command behind the menu or action icon would be called from the server-side, passing the original item's object (with all its functions and private data).

Last commit changes it to work like vs-code. and not move the tree-items' customer objects of the plugin/vs-code extension back and forth at all.

[benoitf]

@ariel-bentu we could get rid of JSONRetrocycle/JSONDecycle as well ?

[ariel-bentu]

I suppose we can remove the decycling then - I will add this in an hour

[vinokurig]

I confirm that it fixes #5121.

[ariel-bentu]

@benoitf since I'm not familiar with the testing infra, and I assume this breaks tests because it is really changing how things work, so the tests needs to be fixed - can you help with that?
Thanks

[vinokurig]

@ariel-bentu I've opened a PR that fixes tests

[ariel-bentu]

Anything else I should do to make this PR get merged?
Thanks

[akosyakov]

looks like a proper solution, please use rebase to integrate changes, we try to keep git history linear and avoid merge commits

[vince-fugnitto]

@marcdumais-work do we need a CQ for the code which is copied?

[ariel-bentu]

No. Eventually I did not copy the code, as we don't need the decycle code. the fix changes the way we handle tree initiated commands.

[benoitf]

@vince-fugnitto there is no copied code

[benoitf]

@ariel-bentu it seems rebase didn't worked well

[vince-fugnitto]

@ariel-bentu it seems rebase didn't worked well

Thanks, I'll wait for the cleanup.
I had a hard time understanding what the final PR would ultimately look like.

[akosyakov]

See https://github.com/theia-ide/theia/pull/5196/files#r287686664

Please squash the history. Change is really small for 7 commits.

[ariel-bentu]

all squashed to 1 commit

[ariel-bentu]

@akosyakov, in command-registry.ts, you can see that when command is executed, it prefers the selected item over the args passed. So, when inline action is triggered, it is not effective to pass the current node id, but instead, I need to set the current item to be the selected item.
So I believe the code I had before, in which I set the global selection before calling the command, is necessary
can you suggest anyting else other than bringing back the code I had before?

Thanks

See code:

   private executeLocalCommand<T>(id: string, ...args: any[]): PromiseLike<T> {
        const handler = this.handlers.get(id);
        if (handler) {
            args = args.map(arg => this.argumentProcessors.reduce((r, p) => p.processArgument(r), arg));
            return Promise.resolve(this.selectionService.selection !== undefined ? handler(this.selectionService.selection) : handler(...args));
        } else {
            return Promise.reject(new Error(`Command ${id} doesn't exist`));
        }
    }

[ariel-bentu]

@akosyakov, I modified it to select the node, otherwise inline actions don't work properly. I assume they don't work also now (prior to this PR), if another item is selected and you hover one non-selected item and click the inline action. the context would be the selected item and not the one you clicked on.

So I added to the onClick of the inline icon this:

const node = this.model.getNode(treeItemId);
            if (node as SelectableTreeNode) {
                this.model.selectNode(node as SelectableTreeNode);
            }

[ariel-bentu]

@akosyakov - can you merge this please before the new version? thanks

[akosyakov]

the minimal fix is here: #5310