Fix Converting circular structure to JSON Error in tree-view #5661

vinokurig · 2019-07-08T09:52:44Z

What it does

Pass id to tree item command arguments like in vscode:

https://github.com/microsoft/vscode/blob/c5cdf50e1708b962382821e782746b06c4cdf53b/src/vs/workbench/api/common/extHostTreeViews.ts#L455

https://github.com/microsoft/vscode/blob/c5cdf50e1708b962382821e782746b06c4cdf53b/src/vs/workbench/api/common/extHostCommands.ts#L236

fixes eclipse-che/che#13790

How to test

Download vscode github pull-request plugin to the theia's plugins directory (create the directory in the root if not present)
Download vscode git extension to the theia's plugins directory (create the directory in the root if not present)
Remove theia's git extension in the examples/browser/package.json
build and start theia
Open a workspace with project cloned from github and contains pull requests
WRONG: pull request items don't expand

Review checklist

as an author, I have thoroughly tested my changes and carefully reviewed in accordance with the review guideline

Reminder for reviewers

each approve has supporting comments in accordance with the review guideline
the approve without a comment should be dismissed

akosyakov · 2019-07-09T07:40:57Z

Please rewrite the description with the following template:

<!--
Thank you for your Pull Request. Please provide a description and review
the requirements below.

Contributors guide: https://github.com/theia-ide/theia/blob/master/CONTRIBUTING.md
-->

#### What it does
<!-- Include relevant issues and describe how they are addressed. -->

#### How to test
<!-- Explain how a reviewer can reproduce a bug, test new functionality or verify perf improvements.  -->

#### Review checklist

- [ ] as an author, I have thoroughly tested my changes and carefully reviewed in accordance with [the review guideline](https://github.com/theia-ide/theia/pull/5616/files#diff-195a635ad245ded9076330e31134bd80R18)

#### Reminder for reviewers

- each approve has supporting comments in accordance with [the review guideline](https://github.com/theia-ide/theia/pull/5616/files#diff-195a635ad245ded9076330e31134bd80R18)
- the approve without a comment should be dismissed

akosyakov · 2019-07-09T07:42:10Z

@tetchel does it address #5666? Could you verify?

tetchel · 2019-07-09T15:56:16Z

This appears to resolve #5666, though the commands still don't work on-click (but that is a separate issue).

akosyakov · 2019-07-10T08:02:42Z

I cannot get how to test section to work. I'm using this branch for testing. Also removed Theia git extension and installed vscode git extension. It should be necessary @vinokurig?

I'm getting following errors in the console:

Uncaught (in promise) TypeError: Cannot read property 'getAPI' of undefined
    at new t.BuiltinGitProvider (/tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at Object.t.registerBuiltinGitProvider (/tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at /tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36
    at Generator.next (<anonymous>)
    at /tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36
    at new Promise (<anonymous>)
    at n (/tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at t.activate (/tmp/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at PluginManagerExtImpl.<anonymous> (/workspace/theia/packages/plugin-ext/lib/plugin/plugin-manager.js:328)
    at step (/workspace/theia/packages/plugin-ext/lib/plugin/plugin-manager.js:47)

Should I do something else?

vinokurig · 2019-07-11T07:05:43Z

@akosyakov Yes, you are right, I forgot to say about the vscode git extension, updated the description.
Looks like the error that you have mentioned is occurred with latest changes, it is not reproduced before 906c4fe. I'll investigate and create an issue.

akosyakov · 2019-07-12T09:01:37Z

@vinokurig looking as well, can be that git extension is not activated before pr review extension

akosyakov · 2019-07-12T09:20:08Z

~~There is a mismatch between vscode and theia plugin APIs:~~ - no see #5661 (comment)

vscode APIs to fetch extension are synchronous: https://github.com/microsoft/vscode/blob/8fa811108f0f0524c473020ef57b6620f6c201e1/src/vs/vscode.d.ts#L9113
theia plugin APIs are asynchronous: https://github.com/theia-ide/theia/blob/5b48e666453c6d90b46e93290d6459e4f0da840a/packages/plugin/src/theia.d.ts#L152

~~With activation events vscode extensions are getting promises instead of extensions.~~ Where are 2 ways to go about it:

~~either align API which is a breaking change for theia plugins~~
or activate plugins only when its dependencies are activated

cc @benoitf @evidolob what will be better?

akosyakov · 2019-07-12T09:32:37Z

Sorry there is no mismatch, confused Plugin with Promise. We need to activate dependencies as well when activation of an extension happens. I will push a commit to this PR.

akosyakov · 2019-07-12T15:01:01Z

@vinokurig please try again, i've pushed a fix to activate dependencies before activating a plugin

akosyakov · 2019-07-12T15:01:53Z

I cannot make this extension work in Gitpod. It asked for authentication but bogus GitHub page is opening. Trying locally...

akosyakov · 2019-07-12T15:13:29Z

Locally I'm getting a new error:

hosted-plugin.ts:20 Uncaught (in promise) TypeError: Cannot read property 'packageJSON' of undefined
    at new t.Telemetry (:3000/private/var/folders/5w/jw7jf9m97mb17ygz25fr1rd40000gp/T/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at :3000/private/var/folders/5w/jw7jf9m97mb17ygz25fr1rd40000gp/T/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36
    at Generator.next (<anonymous>)
    at :3000/private/var/folders/5w/jw7jf9m97mb17ygz25fr1rd40000gp/T/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36
    at new Promise (<anonymous>)
    at n (:3000/private/var/folders/5w/jw7jf9m97mb17ygz25fr1rd40000gp/T/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at t.activate (:3000/private/var/folders/5w/jw7jf9m97mb17ygz25fr1rd40000gp/T/vscode-unpacked/vscode-pull-request-github-0.8.0.vsix/extension/media/extension.js:36)
    at PluginManagerExtImpl.<anonymous> (:3000/Users/kosyakov/git/theia/packages/plugin-ext/lib/plugin/plugin-manager.js:362)
    at step (:3000/Users/kosyakov/git/theia/packages/plugin-ext/lib/plugin/plugin-manager.js:47)
    at Object.next (:3000/Users/kosyakov/git/theia/packages/plugin-ext/lib/plugin/plugin-manager.js:28)

packages/plugin-ext/src/plugin/plugin-manager.ts

akosyakov · 2019-07-12T15:48:44Z

Is there a simple plugin to verify a change? cc @vinokurig I cannot get it work, since it expect to sign in from VS Code and does not seem to allow from other IDE, i.e. we provide theia as env schema and the VS Code PR server does not seem to handle it well. I mean this code https://github.com/microsoft/vscode-pull-request-github/blob/08b9811ce54e8f225307398f6fb08128c53959dc/src/authentication/githubServer.ts#L16-L19

packages/plugin-ext-vscode/src/node/scanner-vscode.ts

akosyakov · 2019-07-15T10:23:50Z

@tetchel Could you provide how to you verify it? I would like to check it again, cannot do it with the PR extension.

tetchel · 2019-07-15T13:45:55Z

I'm using my extension https://github.com/eclipse/codewind-vscode without the workaround I put in here eclipse-archived/codewind-vscode@92b5683

vinokurig · 2019-07-16T14:23:37Z

@akosyakov Sorry for the late response

Is there a simple plugin to verify a change? cc @vinokurig

Unfortunately I don't now a simpler way to reproduce the bug.

I cannot get it work, since it expect to sign in from VS Code and does not seem to allow from other IDE, i.e. we provide theia as env schema and the VS Code PR server does not seem to handle it well. I mean this code https://github.com/microsoft/vscode-pull-request-github/blob/08b9811ce54e8f225307398f6fb08128c53959dc/src/authentication/githubServer.ts#L16-L19

You can authenticate via GitHub token, The plugin provides a command: GitHub Pull Requests: Manually Provide Authentication Response

@vinokurig please try again, i've pushed a fix to activate dependencies before activating a plugin

I've tested the latest changes with GitHub PR Plugin and the bug didn't reproduce.

akosyakov · 2019-07-16T15:58:19Z

@vinokurig ok

I've tried with GitHub Pull Requests: Manually Provide Authentication Response as well. Somehow did not work for me.

Anyway since @tetchel can confirm that it is gone i will approve your part.

We need to get rid of merge commits (rebase should be used to pick up changes), and add a comment in the code for #5661 (review).

tetchel · 2019-07-16T16:02:21Z

It worries me that you can't verify the fix, I will put together a simple plugin for verification later today

tetchel · 2019-07-16T16:43:58Z

I've put together a sample for reproducing this: https://github.com/tetchel/vscode-extension-samples/tree/theia-5561/tree-view-sample

there is a built vsix there you may use.

I edited the File Explorer sample to pass an object to its treeitems' command which contains circular references, which is what causes #5666. You can see that against master this breaks the treeview (the files don't appear when the directories are expanded).

In the logs:

root ERROR [hosted-plugin: 91137] Promise rejection not handled in one second: TypeError: Converting circular structure to JSON
root ERROR [hosted-plugin: 91137] With stack trace: TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
    at Function.MessageFactory.replyOK (/Users/tim/programs/clones/theia/packages/plugin-ext/lib/api/rpc-protocol.js:280:103)
    at /Users/tim/programs/clones/theia/packages/plugin-ext/lib/api/rpc-protocol.js:158:51
    at process._tickCallback (internal/process/next_tick.js:68:7)

After building from this PR, it works again.

So this does fix #5666 :)

tetchel · 2019-07-16T16:45:20Z

I should clarify that clicking/double-clicking the tree items still does not invoke the command, but again that is a separate issue.

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig · 2019-07-17T06:47:16Z

Merged master, fixed conflicts.
GitHub plugin works fine now:

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov

I've verified that:

converting circular structure JSON error is gone with @tetchel example, thank you for an example ❤️
pull request extension is activated after git extension activation

akosyakov · 2019-07-17T07:06:49Z

Let's merge it today if build is green and no one objects.

akosyakov · 2019-07-17T08:10:22Z

Please merge

vinokurig requested review from benoitf and evidolob July 8, 2019 09:52

akosyakov added the vscode issues related to VSCode compatibility label Jul 9, 2019

This comment has been minimized.

Sign in to view

vinokurig force-pushed the che-13790 branch from f4b616b to 7bbe463 Compare July 10, 2019 06:27

akosyakov mentioned this pull request Jul 12, 2019

support workspaceContains activation events #5649

Merged

3 tasks

akosyakov force-pushed the che-13790 branch from 7bbe463 to 57fb86a Compare July 12, 2019 15:00

akosyakov reviewed Jul 12, 2019

View reviewed changes

packages/plugin-ext/src/plugin/plugin-manager.ts Show resolved Hide resolved

akosyakov force-pushed the che-13790 branch from 57fb86a to 2c402d0 Compare July 12, 2019 15:57

benoitf reviewed Jul 15, 2019

View reviewed changes

packages/plugin-ext-vscode/src/node/scanner-vscode.ts Show resolved Hide resolved

[plugin ] Fix Converting circular structure to JSON Error in tree-view

162aaac

Signed-off-by: Igor Vinokur <ivinokur@redhat.com>

vinokurig force-pushed the che-13790 branch from d017195 to cdc3228 Compare July 17, 2019 06:45

[plugin] activate dependencies before activating a plugin

4203fb0

Signed-off-by: Anton Kosyakov <anton.kosyakov@typefox.io>

akosyakov force-pushed the che-13790 branch from cdc3228 to 4203fb0 Compare July 17, 2019 07:04

akosyakov approved these changes Jul 17, 2019

View reviewed changes

evidolob approved these changes Jul 17, 2019

View reviewed changes

benoitf merged commit 6021d79 into master Jul 17, 2019

vinokurig deleted the che-13790 branch July 17, 2019 08:16

This was referenced Jul 17, 2019

Address fixes in theia eclipse-archived/codewind-vscode#122

Closed

TreeItem on click commands don't work #5744

Closed

view containers integration #5665

Merged

This was referenced Aug 8, 2019

Improve QuickInput/InputBox API's #5890

Merged

GitHub plugin doesn't work eclipse-che/che#13789

Closed

bug(regression): starting a task from my workspace view is not starting it ... eclipse-che/che#14204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Converting circular structure to JSON Error in tree-view #5661

Fix Converting circular structure to JSON Error in tree-view #5661

vinokurig commented Jul 8, 2019 •

edited

Loading

akosyakov commented Jul 9, 2019

akosyakov commented Jul 9, 2019

This comment has been minimized.

tetchel commented Jul 9, 2019 •

edited

Loading

akosyakov commented Jul 10, 2019

vinokurig commented Jul 11, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019 •

edited

Loading

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019 •

edited

Loading

akosyakov commented Jul 15, 2019

tetchel commented Jul 15, 2019

vinokurig commented Jul 16, 2019

akosyakov commented Jul 16, 2019 •

edited

Loading

tetchel commented Jul 16, 2019 •

edited

Loading

tetchel commented Jul 16, 2019 •

edited

Loading

tetchel commented Jul 16, 2019

vinokurig commented Jul 17, 2019

akosyakov left a comment

akosyakov commented Jul 17, 2019

akosyakov commented Jul 17, 2019

Fix Converting circular structure to JSON Error in tree-view #5661

Fix Converting circular structure to JSON Error in tree-view #5661

Conversation

vinokurig commented Jul 8, 2019 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

akosyakov commented Jul 9, 2019

akosyakov commented Jul 9, 2019

This comment has been minimized.

tetchel commented Jul 9, 2019 • edited Loading

akosyakov commented Jul 10, 2019

vinokurig commented Jul 11, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019 • edited Loading

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019

akosyakov commented Jul 12, 2019 • edited Loading

akosyakov commented Jul 15, 2019

tetchel commented Jul 15, 2019

vinokurig commented Jul 16, 2019

akosyakov commented Jul 16, 2019 • edited Loading

tetchel commented Jul 16, 2019 • edited Loading

tetchel commented Jul 16, 2019 • edited Loading

tetchel commented Jul 16, 2019

vinokurig commented Jul 17, 2019

akosyakov left a comment

Choose a reason for hiding this comment

akosyakov commented Jul 17, 2019

akosyakov commented Jul 17, 2019

vinokurig commented Jul 8, 2019 •

edited

Loading

tetchel commented Jul 9, 2019 •

edited

Loading

akosyakov commented Jul 12, 2019 •

edited

Loading

akosyakov commented Jul 12, 2019 •

edited

Loading

akosyakov commented Jul 16, 2019 •

edited

Loading

tetchel commented Jul 16, 2019 •

edited

Loading

tetchel commented Jul 16, 2019 •

edited

Loading