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

Align the Comments plugin API with the latest vscode #8539

Merged
merged 1 commit into from
Oct 6, 2020
Merged

Conversation

vinokurig
Copy link
Contributor

@vinokurig vinokurig commented Sep 21, 2020

What it does

Align the Comments plugin API with the latest vscode: https://github.com/microsoft/vscode/blob/42226a9169340756897e12b65045c721af9f860b/src/vs/vscode.d.ts#L11411-L11695

This is dummy implementation, there is an issue to replace it with full implementation: #8492

How to test

  1. Remove "@theia/git": "^1.5.0" from examples/browser/package.json file.
  2. Download vscode-git-1.44.2 and copy it to the plugins folder.
  3. Download the vscode GitHub Pull request plugin and copy it to the plugins folder.
  4. Apply GitHub authentication provider:
  1. Start Theia and sign in the GitHub provider:
    screenshot-serverg0cevid1-che-dev-server-3010 192 168 99 253 nip io-2020 09 23-11_42_23

It should be possible to open a pull-request in the editors view:
screenshot-localhost_3030-2020 09 21-17_07_34

Review checklist

Reminder for reviewers

@vinokurig vinokurig added plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility labels Sep 21, 2020
@benoitf
Copy link
Contributor

benoitf commented Sep 23, 2020

Hi @vinokurig

there is something wrong with the test steps

vscode GitHub Pull request plugin has entry

  "extensionDependencies": [
    "vscode.git"
  ],

but the referenced git extension has package.json

  "name": "vscode-git",
  "publisher": "vscode",

so FQN is vscode.vscode-git (incorrect)

so it's downloading vscode.git@latest which is 1.49.1 and then there are duplicates

If I look at https://open-vsx.org/api/vscode/git/1.44.2/file/vscode.git-1.44.2.vsix

I've

  "name": "git",
  "publisher": "vscode",

@vinokurig
Copy link
Contributor Author

@benoitf I've updated the How to test section and the vscode-git extension. Now it works.

@azatsarynnyy
Copy link
Member

FYI: #8525 is approved, so you can get rid of step 5 in How to test

@vinokurig
Copy link
Contributor Author

@akosyakov @benoitf any updates?

@azatsarynnyy
Copy link
Member

How to test
...
5. Merge the onStartupFinished activation event to Theia, compile and start Theia

@vinokurig is step 5 still mandatory? I believe the steps for testing it can be a bit simpler if this PR is rebased on the master changes 😉

@vinokurig
Copy link
Contributor Author

@azatsarynnyy You are right, I merged it today, updated the description, thank you.

@vinokurig
Copy link
Contributor Author

vinokurig commented Sep 29, 2020

And rebased 😉

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Git clone the github authentication plugin to the plugins folder

Doesn't GitHub Pull Requests VS Code extension already provide GitHub Authentication Provider?

I suppose github authentication plugin has to be compiled first. Because I got
image

After cloning github authentication plugin, I've tried to compile it but:

node_modules/@theia/plugin/src/theia-proposed.d.ts:618:34 - error TS1005: ';' expected.

618         readonly items: readonly TimelineItem[];
                                     ~~~~~~~~~~~~


node_modules/@theia/plugin/src/theia-proposed.d.ts:618:47 - error TS1011: An element access expression should take an argument.

618         readonly items: readonly TimelineItem[];
                                                  


node_modules/@theia/plugin/src/theia-proposed.d.ts:679:1 - error TS1128: Declaration or statement expected.

679 }
    ~


error Command failed with exit code 2.

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After cloning github authentication plugin, I've tried to compile it but:

I experienced the same issue trying to compile the provided plugin.

@vinokurig
Copy link
Contributor Author

@azatsarynnyy

Doesn't GitHub Pull Requests VS Code extension already provide GitHub Authentication Provider?

No, vscode has its own built-in GitHub authentication provider. In Che we are going to have such provider in the github-auth plugin.

I suppose github authentication plugin has to be compiled first.

Yes, it must be compiled before the theia start.

@azatsarynnyy @vince-fugnitto

After cloning github authentication plugin, I've tried to compile it but: ...

Sorry, I forgot to mention that those errors do not prevent the plugin from working properly.
I don't want to spend time on it because its just a sample plugin that is not going to be merged anywhere.
I updated the description with it.

@vinokurig
Copy link
Contributor Author

@azatsarynnyy @vince-fugnitto Could you please review it again?

@azatsarynnyy
Copy link
Member

Thanks @vinokurig!

I opened a workspace with Theia sources. It shows me that the plugin activation is in progress and never ends
image

Also, GitHub view is empty
image

@azatsarynnyy
Copy link
Member

When I'm switching to GitHub view

root ERROR TypeError: Cannot use 'in' operator to search for 'uri' in undefined
    at http://localhost:3000/38.bundle.js:1008:51
    at http://localhost:3000/bundle.js:139879:33
    at CallbackList.../../packages/core/lib/common/event.js.CallbackList.invoke (http://localhost:3000/bundle.js:139894:39)
    at Emitter.../../packages/core/lib/common/event.js.Emitter.fire (http://localhost:3000/bundle.js:140021:29)
    at SelectionService.set (http://localhost:3000/bundle.js:144341:44)
    at TreeViewWidget.../../packages/core/lib/browser/tree/tree-widget.js.TreeWidget.updateGlobalSelection (http://localhost:3000/bundle.js:133615:41)
    at TreeViewWidget.../../packages/core/lib/browser/tree/tree-widget.js.TreeWidget.doFocus (http://localhost:3000/bundle.js:133726:18)
    at HTMLDivElement.<anonymous> (http://localhost:3000/bundle.js:134314:78)
    at TreeViewWidget.../../packages/core/lib/browser/tree/tree-widget.js.TreeWidget.onActivateRequest (http://localhost:3000/bundle.js:133712:19)
    at TreeViewWidget.../../node_modules/@phosphor/widgets/lib/widget.js.Widget.processMessage (http://localhost:3000/bundle.js:27117:22)

@vinokurig
Copy link
Contributor Author

@azatsarynnyy @vince-fugnitto
Looks like you haven't sign in the GitHub authentication provider, see item 5 of the description. The error is not reproduced if the GitHub plugin is authenticated.

@azatsarynnyy
Copy link
Member

Looks like you haven't sign in the GitHub authentication provider, see item 5 of the description. The error is not reproduced if the GitHub plugin is authenticated.

It seems so...
Now I can see the PR
image

Shouldn't it warn the user in the UI when one didn't sign in the GitHub authentication provider?
Instead of just logging an error root ERROR TypeError: Cannot use 'in' operator to search for 'uri' in undefined

@vinokurig
Copy link
Contributor Author

@azatsarynnyy

Shouldn't it warn the user in the UI when one didn't sign in the GitHub authentication provider?
Instead of just logging an error root ERROR TypeError: Cannot use 'in' operator to search for 'uri' in undefined

This PR is about updating existed dummy API with changed interfaces from vscode. The GitHub plugin mentioned here is just an example to show that it works better with this changes. Looks like this error is caused by an empty tree which is shown instead of login panel:
screenshot-github com-2020 10 05-10_43_40
This error should be fixed by #7178.

@vinokurig
Copy link
Contributor Author

@azatsarynnyy @vince-fugnitto any updates?

@benoitf
Copy link
Contributor

benoitf commented Oct 6, 2020

I will test right now

Copy link
Contributor

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works as expected
I'm able to use github PR plug-in

image

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that it works with the steps described:

image

Do we need to update the changelog with the breaking changes (renaming of namespaces, methods)?

@benoitf
Copy link
Contributor

benoitf commented Oct 6, 2020

I think that changelog breaking change is kind of optional as

  • people will expect that theia match the VS Code API change
  • I'm not sure we ever introduce changelog for VS Code API compliance changes (only for Theia extensions)

@vinokurig
Copy link
Contributor Author

I don't think that it has to be mentioned in the changelog because this is just an update of the existing dummy API.

@vince-fugnitto
Copy link
Member

I don't think that it has to be mentioned in the changelog because this is just an update of the existing dummy API.

No problem, just wanted to bring it up if it was something we wanted to add.

@vinokurig vinokurig merged commit 56c158d into master Oct 6, 2020
@vinokurig vinokurig deleted the theia-8492 branch October 6, 2020 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants