Skip to content
This repository has been archived by the owner on Apr 4, 2023. It is now read-only.

feat(namespace): Export @eclipse-che/plugin namespace using VS Code extension mechanism #994

Merged
merged 1 commit into from
Feb 24, 2021

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Feb 15, 2021

What does this PR do?

Export @eclipse-che/plugin namespace through export of VS Code extension export API

clients could lookup getExtension('@eclipse-che.ext-plugin') to grab this namespace

Screenshot/screencast of this PR

oxrKWYn0wV.mp4

What issues does this PR fix or reference?

eclipse-che/che#18837

How to test this PR?

Write an example using getExtension('@eclipse-che/ext-plugin') and then for example grab workspace details.
I've created a vscode extension doing this job

const eclipseCheExtPlugin = vscode.extensions.getExtension('@eclipse-che.ext-plugin');
if (eclipseCheExtPlugin) {
  // grab user
  const user = await eclipseCheExtPlugin.exports.user.getCurrentUser();
 vscode.window.showInformationMessage(`Eclipse Che user information: id ${user.id} with name ${user.name}`);
} else {
  vscode.window.showWarningMessage('Not running inside che, not displaying any che user information');
}

Here to try this VS Code extensionon che.openshift.io
https://che.openshift.io/f?url=https://gist.githubusercontent.com/benoitf/b646af9e1cb9aacc23e3e588e413af47/raw/1f42b0a67bc40ed927a4215e26252fc12c9a3e58/devfile.yaml

or workspaces.openshift.com
https://workspaces.openshift.com/f?url=https://gist.githubusercontent.com/benoitf/b646af9e1cb9aacc23e3e588e413af47/raw/1f42b0a67bc40ed927a4215e26252fc12c9a3e58/devfile.yaml

bring the command palette and search for a Test command

PR Checklist

As the author of this Pull Request I made sure that:

Reviewers

Reviewers, please comment how you tested the PR when approving it.

Happy Path Channel

HAPPY_PATH_CHANNEL=next

Change-Id: I4d8d6c49b6797bd98b76486127ec3e38f243e120
Signed-off-by: Florent Benoit fbenoit@redhat.com

@che-bot
Copy link
Contributor

che-bot commented Feb 15, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:994
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:994

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@tsmaeder
Copy link
Contributor

tsmaeder commented Feb 16, 2021

Why would a plugin using that namespace not simply require the namespace at compilation time?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 16, 2021

@tsmaeder because it's not only 'compilation time'

@tsmaeder
Copy link
Contributor

@benoitf I still don't get it. Can you give an example?

@benoitf
Copy link
Contributor Author

benoitf commented Feb 16, 2021

@tsmaeder example is given in the attached issue.

@fbricon
Copy link

fbricon commented Feb 16, 2021

When testing this solution, I was able to properly identify Che user with the exposed API, for telemetry purposes

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.

I've tested it with the provided example. Everything works as described. 👍

@@ -18,6 +18,9 @@
"@theia/plugin": "next",
"@theia/plugin-packager": "latest"
},
"extensionDependencies": [
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I didn't get why the Workspace Plug-in has to depend on it?
Could you please elaborate a bit?

If it's just for bootstrapping, isn't the * activation event enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's to make sure that it's loaded as first plug-ins because there is no concept of optional dependency in VScode api

plugins/ext-plugin/README.md Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@ sources:
- extensions/eclipse-che-theia-remote-impl-che-server
plugins:
- plugins/containers-plugin
- plugins/ext-plugin
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something more descriptive? E.g. che-api-export-plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name of the folder can be as long as you want (can change it easily) but for the name of the plug-in it looks a bit too long/verbose for consumers.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, it's important to have the name as short as possible here. I just forgot it's supposed to be used by the external VS Code extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you've a better short name suggestion, I can rename it

Copy link
Member

Choose a reason for hiding this comment

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

It's good. Nevermind )

Copy link
Contributor

@tsmaeder tsmaeder left a comment

Choose a reason for hiding this comment

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

After inspecting the vsix provided by @fbricon I'm convinced the vsix is not properly packaged (see comment on the issue). Until we clear this up, I don't think we should merge a workaround.

@benoitf benoitf force-pushed the CHE-18837 branch 2 times, most recently from 1c5d5f7 to 31b73f5 Compare February 17, 2021 08:21
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.

All my notes have been addressed. Thanks!

@che-bot
Copy link
Contributor

che-bot commented Feb 17, 2021

❌ E2E Happy path tests failed ❗

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:994
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:994

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@azatsarynnyy
Copy link
Member

Please, see my comments on eclipse-che/che#18837 (comment)

@benoitf
Copy link
Contributor Author

benoitf commented Feb 19, 2021

@tsmaeder this PR is not about fixing a bug or providing a workaround.
It's to simplify users that want to use Che API from a VS Code extension without even importing a 3rd party module.

About lacking of type checking at compilation time, while developers coming from Java world may like it, VS Code extensions may be written in javascript and not in typescript. And again we don't remove the current way of importing namespace.

It's a free will for developers, they should have the choice.

@tsmaeder
Copy link
Contributor

@benoitf I've understood that by now: if this is a community request, I'm fine with it. Can you tell me if this is something clients requested? I was going to talk to @fbricon when he gets back on monday. My impression was he just wants "something that works".

@benoitf
Copy link
Contributor Author

benoitf commented Feb 19, 2021

It's when I saw how people struggle on how to import a namespace that I added this improvement so that in a couple of seconds you have something that works immediately. Nothing to debug/understand.

…xtension mechanism

Change-Id: I4d8d6c49b6797bd98b76486127ec3e38f243e120
Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@codecov
Copy link

codecov bot commented Feb 23, 2021

Codecov Report

Merging #994 (01ff09a) into master (88ead27) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #994      +/-   ##
==========================================
+ Coverage   57.93%   58.02%   +0.08%     
==========================================
  Files          45       46       +1     
  Lines        2028     2032       +4     
  Branches      333      333              
==========================================
+ Hits         1175     1179       +4     
  Misses        843      843              
  Partials       10       10              
Impacted Files Coverage Δ
plugins/ext-plugin/src/ext-plugin.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88ead27...01ff09a. Read the comment docs.

@che-bot
Copy link
Contributor

che-bot commented Feb 23, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:994
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:994

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@che-bot
Copy link
Contributor

che-bot commented Feb 23, 2021

✅ E2E Happy path tests succeed 🎉

Try Che-Theia editor only Try Che-Theia with Java/maven example Try Che-Theia with NodeJs example

See Details

name link
che-theia docker.io/maxura/che-theia:994
che-theia-endpoint-runtime-binary docker.io/maxura/che-theia-endpoint-runtime-binary:994

Tested with Eclipse Che Single User on K8S (minikube v1.1.1)

  • Use comment "[crw-ci-test]" to rerun happy path E2E test.
  • Use comment "[crw-ci-test --rebuild]" to re-build the images and rerun happy path E2E test.

@ericwill
Copy link
Contributor

Let's merge 👍

@benoitf please do us the honours

@benoitf
Copy link
Contributor Author

benoitf commented Feb 24, 2021

doc PR merged, merging there.

@benoitf benoitf merged commit 4e9f582 into eclipse-che:master Feb 24, 2021
@benoitf benoitf deleted the CHE-18837 branch February 24, 2021 12:27
@che-bot che-bot added this to the 7.27 milestone Feb 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants