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

Add missing vscode api in env namespace #11446

Merged
merged 3 commits into from
Jul 28, 2022

Conversation

yiningwang11
Copy link
Contributor

@yiningwang11 yiningwang11 commented Jul 20, 2022

What it does

Fixes: #10662

The pull-request adds support for the env namespace according to the vscode API:

  • appHost
  • isNewAppInstall
  • isTelemetryEnabled
  • onDidChangeTelemetryEnabled
  • removeName

How to test

  1. include the following test extension (env-test.zip)
  2. confirm for both the browser and electron that the notifications for each API is supported

Using the electron app, notifications indicating the value of those APIs would pop up.The appHost is set to 'desktop', the remoteName is set to 'undefined'. For isNewAppInstall, we use the creation date of a random theia file as the installation date. Moreover, since theia doesn't support telemetry, we implemented stubs for the API related to telemetry. The isTelemetryEnabled API is set to false. The onDidChangeTelemetryEnabled always fires an empty event.

Review checklist

Reminder for reviewers

Added appHost, isNewAppInstall, isTelemetryEnabled, onDidChangeTelemetryEnabled, and remoteName API to our missing vscode API implementation.

For isNewAppInstall, we use the creation date of a random theia file as installation date. Moreover, since theia doesn't support telemetry, we implemented stubs for the API related to telemetry.
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

@yiningwang11 Thank you for your first contribution!

Please make sure to sign the ECA with the same email as your commit in order for us to accept your changes.

packages/plugin-ext/src/plugin/env.ts Outdated Show resolved Hide resolved
@vince-fugnitto vince-fugnitto added the vscode issues related to VSCode compatibility label Jul 20, 2022
@JonasHelming
Copy link
Contributor

@yiningwang11 Is this ready for a re-review? I see that the ECA succeeds now.

Follow feedback on `env.appHost` and take the current runtime into
account: `desktop` when running Electron, `web` otherwise.
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.

The changes look good to me and work well 👍
I confirm for both the browser and electron the values are correct.

@JonasHelming
Copy link
Contributor

@vince-fugnitto : Can we merge this for the release still?

@paul-marechal paul-marechal merged commit cf1b71e into eclipse-theia:master Jul 28, 2022
@paul-marechal
Copy link
Member

@JonasHelming the changes were low risk enough to be merged in our opinion, yes :)

@vince-fugnitto vince-fugnitto added this to the 1.28.0 milestone Jul 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement env namespace VSCode API
5 participants