eclipse-theia / theia

Eclipse Theia is a cloud & desktop IDE framework implemented in TypeScript.
http://theia-ide.org
Eclipse Public License 2.0
19.96k stars 2.5k forks source link

VSCode extension icons are not loaded when a path is added to base URL #9596

Open kumbham opened 3 years ago

kumbham commented 3 years ago

Bug Description:

Steps to Reproduce:

  1. Run sample browser-app with a vscode extension. I'm using docker for vscode extension which I got from the public github.
  2. Open http://localhost:3000
  3. Plugin will be loaded with the icon on the activity bar: Screen Shot 2021-06-15 at 5 49 14 PM
  4. Open http://localhost:3000/index.html , you'll observe that icon wouldn't be loaded. Screen Shot 2021-06-15 at 8 55 29 PM Network call:

Request URL: http://localhost:3000/index.html/hostedPlugin/docker_vscode/resources/activity-bar-icon.svg Request Method: GET Status Code: 404 Not Found

If you see the above get URL, extension is using http://localhost:3000/index.html as the base URL to fetch the extension assets. How do we configure theia to ignore index.html in the path while fetching the assets for an extension?

Additional Information

colin-grant-work commented 3 years ago

It looks like there are likely a number of places in the application where we assume that index.html will not be included in the path. When I add index.html to the path, I end up unable to even load the application, with backend logs

root ERROR Cannot find a ws handler for the path: /index.html/services
root ERROR Cannot find a ws handler for the path: /index.html/services
root ERROR Cannot find a ws handler for the path: /index.html/services

Which seems to come from the message contribution. I.e. no interaction between the front and back-ends on the /services path will suceed.

Is there a particular reason you do want to include index.html in your path?

vince-fugnitto commented 3 years ago

@colin-grant-work I was just about to write the same thing, the path http://localhost:3000/index.html is not valid.

@kumbham I'm not sure how you achieved step 4, can you test with master, it looks like you are using the Cloud Shell Editor and not the example-browser as you described in step 1.

kumbham commented 3 years ago

@colin-grant-work @vince-fugnitto thanks for the reply. here are my implementation details:

I'm running theia on a remote machine behind a proxy that allows requests with certain paths. localhost:3000/ is too generic so I thought I could whitelist index.html on the proxy. I'm displaying theia within an iframe which points to http://remote-machine-proxy/index.html. Using 0.11.0 versions of theia extensions, I was able to load the editor. The only issue I found till now is with vscode extension icons. If this is not advised, I would remove index.html iframe src path.

AmeyaLokre commented 2 years ago

@vince-fugnitto @colin-grant-work , do you have any feedback based on the kumbham's reply ?

vince-fugnitto commented 2 years ago

@vince-fugnitto @colin-grant-work , do you have any feedback based on the kumbham's reply ?

@kumbham I don’t believe I have any additional comments or feedback at this time, whitelisting index.html will likely not work due to the reasons already expressed in https://github.com/eclipse-theia/theia/issues/9596#issuecomment-864089326.

AmeyaLokre commented 2 years ago

@kumbham , can you clarify once how did you add the docker for vscode extension in the sample browser-app ? Was it by adding a dependency

"dependencies": {
  .
  .
  "docker": "latest"
}

or theiaPlugin

"theiaPlugins": {
  "vscode-builtin-docker": "https://open-vsx.org/api/vscode/docker/1.52.1/file/vscode.docker-1.52.1.vsix"
}

in package.json file of browser-app ?

AmeyaLokre commented 2 years ago

@vince-fugnitto @colin-grant-work , is there a way to override the method toExternalIconUrl https://github.com/eclipse-theia/theia/blob/82c847d4303/packages/plugin-ext/src/main/browser/plugin-shared-style.ts#L123 ?

Is there a specific reason why this method is defined as static ?

AmeyaLokre commented 2 years ago

@kittaakos @akosyakov

msujew commented 2 years ago

@AmeyaLokre There is, you can simply statically override it:

PluginSharedStyle.toExternalIconUrl = (x: string): string => '';

There doesn't seem to be a good reason for this to be static however. We should probably change that.

AmeyaLokre commented 2 years ago

@msujew thanks for your response! Can you share an example of overriding a similar static method ?

msujew commented 2 years ago

@AmeyaLokre You mean a more elaborate example for overriding the PluginSharedStyle.toExternalIconUrl? What do you mean with a "similar" static method? Like this?

PluginSharedStyle.toExternalIconUrl = (iconUrl: string): string => {
    if (iconUrl.startsWith('hostedPlugin/')) {
        return new Endpoint({ path: iconUrl }).getRestUrl().toString();
    }
    return iconUrl;
}
AmeyaLokre commented 2 years ago

@msujew . Thanks, this helps. How can I call the un-overriden method in the overriden method ?

PluginSharedStyle.toExternalIconUrl = (iconUrl: string): string => {
    const baseIconUrl = PluginSharedStyle.toExternalIconUrl(iconUrl);
    return baseIconUrl.replace("abc", "");
}
msujew commented 2 years ago

@AmeyaLokre You can store it somewhere for later use:

const original = PluginSharedStyle.toExternalIconUrl;
PluginSharedStyle.toExternalIconUrl = (iconUrl: string): string => {
    return original(iconUrl).replace(...);
}
AmeyaLokre commented 2 years ago

@msujew , its a good idea. although the variable original is not saving the initial implementation of the method toExternalIconUrl.

As you mentioned before, there is no need for the method toExternalIconUrl to be static. When will the static keyword be removed from the method ?