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

[vscode] add builtins to example applications #6883

Merged
merged 1 commit into from
Jan 21, 2020
Merged

Conversation

vince-fugnitto
Copy link
Member

@vince-fugnitto vince-fugnitto commented Jan 14, 2020

What it does

The pull request updates both example applications (browser and electron) to use VS Code builtin extensions, and removes language specific extensions which will ultimately be deprecated.
The main idea is to easily develop and test against the builtin extensions and also quickly resolve compatibility issues with VS Code. Missing functionality should be replaced by appropriate and license certified external plugins.

The following builtins have not been added due to known issues/limitations:

Plugin Reason
vscode-builtin-css-language-features #6623
vscode-builtin-html-language-features #6623
vscode-builtin-json-language-features #6623, #6647
vscode-builtin-debug-server-ready #6628
vscode-builtin-php #6672
vscode-builtin-php-language-features #6672
vscode-builtin-extension-editing #6634, #3983
vscode-builtin-image-preview #6636
vscode-builtin-git -
vscode-builtin-git-ui -

VS Code external extensions needed to support removed functionality:

  • @theia/editorconfig > editorconfig - we will likely need to publish our own release
  • @theia/tslint > tslint or even better eslint

Todo:

  • refactor and generalize download-debug-adapters, and move it to @theia/cli
  • find and add appropriate external VS Code extensions to compliment deprecated functionality
  • update changelog
  • add node and node2 extensions
    • package node ourselves since they do not have a .vsix release
    • package node2 ourselves since they do not have a .vsix release
  • verify builtin licenses (create a script and verify through clearlydefined)

How to test

Verify that CI successfully passes (examples should no longer pull removed extensions).

Review checklist

Reminder for reviewers

Signed-off-by: vince-fugnitto vincent.fugnitto@ericsson.com

@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Jan 14, 2020
@vince-fugnitto vince-fugnitto self-assigned this Jan 14, 2020
@vince-fugnitto
Copy link
Member Author

@akosyakov I'm not sure if for a first step we want to find external VS Code extensions (ex: editorconfig, tslint/eslint) and add them to the examples, or if it is in fact unnecessary.

@akosyakov
Copy link
Member

I see that git, git-ui and merge-conflicts are skipped. For first 2 it is fine for first iteration, since many clients are prefer using Theia extensions instead, but merge-conflicts can be used instead of Theia extension. Did you skip something else? Could you list skipped in the description please?

@akosyakov
Copy link
Member

I'm not sure if for a first step we want to find external VS Code extensions (ex: editorconfig, tslint/eslint) and add them to the examples, or if it is in fact unnecessary.

@vince-fugnitto If it does not harm dog fooding somehow (i.e. files get reformatted constantly without editorconfig, pushed code is bogus because linting errors are missing) then we call do it later, just file a follow-up issue. btw eslint extension can handle our tslint rules? if not then we should stick with tslint extension for now.

@vince-fugnitto vince-fugnitto force-pushed the vf/builtins branch 4 times, most recently from 9d1066a to 007c2b5 Compare January 15, 2020 23:03
@akosyakov
Copy link
Member

@vince-fugnitto please list what are reasons to exclude:

@theia/vscode-builtin-markdown-language-features
@theia/vscode-builtin-php-language-features
@theia/vscode-builtin-typescript-language-features
@theia/vscode-builtin-git
@theia/vscode-builtin-git-ui

We have to include @theia/vscode-builtin-typescript-language-features and it would be good to include @theia/vscode-builtin-php-language-features.

@akosyakov akosyakov added the dependencies pull requests that update a dependency file label Jan 16, 2020
@akosyakov
Copy link
Member

I think we also should add a record to CHANGELOG saying which extensions were deprecated and how to switch to built-in extensions.

@vince-fugnitto
Copy link
Member Author

@vince-fugnitto please list what are reasons to exclude:

@theia/vscode-builtin-markdown-language-features
@theia/vscode-builtin-php-language-features
@theia/vscode-builtin-typescript-language-features
@theia/vscode-builtin-git
@theia/vscode-builtin-git-ui

We have to include @theia/vscode-builtin-typescript-language-features and it would be good to include @theia/vscode-builtin-php-language-features.

I will add `@theia/vscode-builtin-typescript-language-features 👍

I did not add @theia/vscode-builtin-php-language-features since the base @theia/vscode-builtin-php had issues. If you think they should be added I can do so.

As for git and git-ui, do you think it should be added now or at a later time?
I guess we need to see what to do with our git-history which ARM was keen on keeping.

package.json Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto force-pushed the vf/builtins branch 3 times, most recently from 5a8f7c4 to 1402d42 Compare January 16, 2020 16:20
@vince-fugnitto vince-fugnitto added the builtins Issues related to VS Code builtin extensions label Jan 16, 2020
.travis.yml Show resolved Hide resolved
@akosyakov
Copy link
Member

I did not add @theia/vscode-builtin-php-language-features since the base @theia/vscode-builtin-php had issues. If you think they should be added I can do so.

ok, please update the PR description, it does not say reference any issue and seems to say that some extensions are excluded although they are included

As for git and git-ui, do you think it should be added now or at a later time?
I guess we need to see what to do with our git-history which ARM was keen on keeping.

yes, we have to figure out how we go with git, not everybody want to drop it yet

@vince-fugnitto vince-fugnitto force-pushed the vf/builtins branch 5 times, most recently from 66979e7 to ffb7716 Compare January 17, 2020 16:21
@vince-fugnitto vince-fugnitto marked this pull request as ready for review January 17, 2020 16:33
@vince-fugnitto vince-fugnitto force-pushed the vf/builtins branch 2 times, most recently from 9e04b8b to 95e4665 Compare January 17, 2020 16:53
package.json Outdated Show resolved Hide resolved
@akosyakov
Copy link
Member

It looks good to me. @eclipse-theia/ecd-theia-committers Does anyone have objections?

  • We will keep old extensions till theia-apps are migrated and then remove them.
  • We may add java and python vscode extensions later if we will need them often.

@marcdumais-work Do we need any CQ process for vscode extensions? We don't distribute them, checking that they are under the compatible license by ourself should be alright, right?

- adds the vscode `builtin` extensions to the example applications (browser, and electron).
- missing extension functionality will need to be replaced by appropriate vscode extensions.

Signed-off-by: vince-fugnitto <vincent.fugnitto@ericsson.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I’ve tested new extensions, language coloring, typescript editing and node debugging work well.

@vince-fugnitto
Copy link
Member Author

@marcdumais-work Do we need any CQ process for vscode extensions? We don't distribute them, checking that they are under the compatible license by ourself should be alright, right?

I verified the dependencies present in the builtin extensions from VS Code and everything looks fine 👍 I described the process in #6926 and plan to productify it in the future so we can re-use it if need be.

@marcdumais-work
Copy link
Contributor

@marcdumais-work Do we need any CQ process for vscode extensions?
checking that they are under the compatible license by ourself should be alright, right?

Yes, like for npm dependencies, we need to do this recursively for all dependencies pulled by the built-ins as well.

I think we can do it ourselves, using a slightly adapted version of the self-check process we use for runtime npm dependencies. We just need to extract all the 1st level npm dependencies of the built-ins, and then we can follow the usual self-check process as documented in the wiki. This will need to be done each time we update the built-ins or other VS Code extensions we may have in the main repo.

We don't distribute them,

According to the Eclipse Foundation we do (once added in this PR), the same way we "distribute" runtime npm dependencies: by having Theia users pull them when e.g. they build this repo or build a Theia-based application.

@akosyakov
Copy link
Member

Merging?

@vince-fugnitto
Copy link
Member Author

Merging?

Merging! Thank you for your help @akosyakov @marcdumais-work 🍾

"@types/requestretry": "^1.12.3",
"mkdirp": "^0.5.0",
"requestretry": "^3.1.0",
"tar": "^4.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

missing @types/tar ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that's correct. I added it as part of #6933 once I realized it was missing, but I will extract it to a different pull request:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins Issues related to VS Code builtin extensions dependencies pull requests that update a dependency file vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants