-
Notifications
You must be signed in to change notification settings - Fork 111
Support webview in dedicate container #947
Conversation
[crw-ci-test --rebuild] |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
|
||
export class RemoteWebview extends WebviewImpl {} | ||
|
||
export class WebviewsContentAware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably will need to spend some times on bringing DI in upstream for the plug-in host
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to digg into this thing and from what I saw, this requires some refactoring in plugin initializing mechanism. Am I right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
|
||
// Browser part perform preprocess initial html content by replacing vscode-resource:/path/to/file | ||
// to https://authority/webview/theia-resource/file/path/to/file. To be able to operate with custom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't for example just patch webview/normalizeRequestUri https://github.com/eclipse-theia/theia/blob/fd0a8ce093273011ecf4500d673a982854af9e55/packages/plugin-ext/src/main/browser/webview/webview.ts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this solution, but it seems, that this won't work. The problem is extending the WebviewWidget won't get a proper effect, because this code is run on browser part on Theia sidecar and we need to provide the container name from environment variable on the sidecar from plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this solution, but it seems, that this won't work.
sorry @vzhukovs but it's not clear to me if
you think that this will not work OR you have tried and this didn't work
Could you please clarify it? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried. The main problem here is that rebound class will be executing on Theia's container where we don't have any sidecar specific information. As far as rebound class will execute from the browser part we also don't have ability to get environment information. So it better to intercept this method's calls from server side on sidecar directly:
VS Code extension (sidecar server side) > Method intercept and link modification (sidecar server side) > Plugin Ext (sidecar server side > theia browser side) > Theia (theia browser side).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can't only rebind only in sidecar usecase ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I saw, I couldn't get this worked
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[crw-ci-test --rebuild] |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
The image that provided in how to test section is conjunction of #946, eclipse-theia/theia#8832 and eclipse-theia/theia#8833 code base. So additional console output is might be in case. But this PR doesn't have additional console output logs. It was done just for the debug purposes. |
@vzhukovs I'm able to reproduce the error constantly on Hosted Che with the Devfile you provided in |
It's not related to the PR changes. The same error is present in master branch. |
There was a problem hiding this 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 according to the provided How to test
steps.
I was able to get the Didact tutorial rendered correctly, which is based on a Webview from a separate sidecar.
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Single User on K8S (minikube v1.1.1)
|
What does this PR do?
This changes proposal adds an ability to run extension that provides webviews in dedicate container. The problem was in resource loading mechanism, that didn't support resource loading by file service from remote containers. This changes proposal modifies webview creation step by providing custom scheme:
file-sidecar-${machineName}
instead of defaultfile
forvscode-resource
ortheia-resource
resources.Depends on:
Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com
Screenshot/screencast of this PR
Provided steps to test.
What issues does this PR fix or reference?
eclipse-che/che#16870
How to test this PR?
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.
Happy Path Channel
HAPPY_PATH_CHANNEL=stable