Skip to content

frontend-plugin: fix creating a webworker for frontend plugin (browser only)#9715

Merged
vitaliy-guliy merged 11 commits intomasterfrom
fix-frontend-plugins
Jul 26, 2021
Merged

frontend-plugin: fix creating a webworker for frontend plugin (browser only)#9715
vitaliy-guliy merged 11 commits intomasterfrom
fix-frontend-plugins

Conversation

@vitaliy-guliy
Copy link
Contributor

@vitaliy-guliy vitaliy-guliy commented Jul 8, 2021

Signed-off-by: Vitaliy Gulyy vgulyy@redhat.com

What it does

After moving to webpack 5, frontend plugins were not working for some time. Now it's not necessary to create a worker-loader.
It's enough to create a worker, pointing on javascript file.
https://webpack.js.org/guides/web-workers/

It's still not possible to create a worker for electron browser. Frontend plugins were turned off and the appropriate check for the browser was added to the hosted-plugin.ts.

Partially solves #6601

How to test

  • Clone and build Theia
  • Create a hello-world theia frontend plugin
  • Run browser version of Theia with the plugin

Review checklist

Reminder for reviewers

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@vitaliy-guliy vitaliy-guliy marked this pull request as ready for review July 12, 2021 13:03
@vitaliy-guliy vitaliy-guliy changed the title frontend-plugin: fix creating a webworker for frontend plugin frontend-plugin: fix creating a webworker for frontend plugin (browser only) Jul 13, 2021
Co-authored-by: Chris Wilson <titanicswimteam@hotmail.com>
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.

I confirmed with the hello-world theia plugin that frontend plugins work again for the browser use-case 👍

loader: 'worker-loader',
options: {
filename: 'worker-ext.[fullhash].js'
filename: 'worker-ext.js'
Copy link
Member

Choose a reason for hiding this comment

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

As I understand it, including the hash will make sure we're not referencing stale code from the cache. Can we find a way to preserve that feature?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing is impossible, but since we don't need a worker-loader we need to keep somewhere the file name.
Before it was inside generated worker-loader script

  return new Worker(__webpack_require__.p + "worker-ext.5f6918e41dd2b417d716.js");

For now we need to keep somewhere a proper name of the file and fetch it before running a worker.

Btw, other javascript files don't have hash in their names. Do we really need to have it only for this specific file?

Copy link
Member

@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.

I think we should turn off front-end plugins more cleanly in the electron case. Beyond that, I there are a couple of changes I just don't understand. It would help if you described hat the fix is a bit in the PR description.

vitaliy-guliy and others added 3 commits July 14, 2021 11:14
Co-authored-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Co-authored-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
Co-authored-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@tsmaeder tsmaeder dismissed their stale review July 16, 2021 13:32

Don't want the PR to be blocked while I'm on vacation

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@paul-marechal
Copy link
Member

@vitaliy-guliy why should we avoid using worker-loader?

Modifying the generated Webpack configuration like you did will not work if we don't include @theia/plugin-ext right?

@vitaliy-guliy
Copy link
Contributor Author

@vitaliy-guliy why should we avoid using worker-loader?

With Webpack 5 we can use webworker without worker loader https://webpack.js.org/guides/web-workers/

I've tried to use it with loader and create a worker using a require as it was before, but it does not work.
Nothing happens as well if I run worker-loader

new Worker(worker-loader script)

Modifying the generated Webpack configuration like you did will not work if we don't include @theia/plugin-ext right?

Yes, it will fail, plugin-ext must be included. I will think how to improve it and make the build successful without plugin-ext.

@vitaliy-guliy
Copy link
Contributor Author

@paul-marechal I played with dependencies a bit, but was easily compile the browsrer/electron.
I think it's because plugin-ext is still physically present on the file system and could be found when running webpack.

@paul-marechal
Copy link
Member

paul-marechal commented Jul 21, 2021

I think it's because plugin-ext is still physically present on the file system and could be found when running webpack.

So it only works in the Theia repo but will most likely fail in a downstream Theia app.

With Webpack 5 we can use webworker without worker loader https://webpack.js.org/guides/web-workers/

I've tried to use it with loader and create a worker using a require as it was before, but it does not work.
Nothing happens as well if I run worker-loader

new Worker(worker-loader script)

I played around with what the Webpack doc was saying: https://github.com/eclipse-theia/theia/compare/mp/frontend-plugins#diff-6ccaef7f7e0ffcd2911ccf76fd97fbe9f2e75f1009ddda5c9024d9807bb093a3R30-R35

It worked for me without the changes to the application-manager generators.

@vitaliy-guliy
Copy link
Contributor Author

vitaliy-guliy commented Jul 22, 2021

I played around with what the Webpack doc was saying: https://github.com/eclipse-theia/theia/compare/mp/frontend-plugins#diff-6ccaef7f7e0ffcd2911ccf76fd97fbe9f2e75f1009ddda5c9024d9807bb093a3R30-R35

It worked for me without the changes to the application-manager generators.

I always get an error, when I try to create an URL with import.meta.url

packages/plugin-ext/src/hosted/browser/plugin-worker.ts:30:52 - error TS1343: The 'import.meta' meta-property is only allowed when the '--module' option is 'esnext' or 'system'.

30         const workerURI = new URL('./worker-main', import.meta.url);
                                                      ~~~~~~~~~~~

That's why I used location.href instead.
But seems // @ts-expect-error (TS1343) helps to compile ignoring the error.

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>
@vitaliy-guliy
Copy link
Contributor Author

@paul-marechal I've updated the PR, based on your branch. Seems it works fine.
Thanks!

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM!

@paul-marechal
Copy link
Member

paul-marechal commented Jul 22, 2021

That's why I used location.href instead.
But seems // @ts-expect-error (TS1343) helps to compile ignoring the error.

Did it work with location.href? I only used import.meta.url because I thought it is a pattern Webpack is looking for to do the proper transformations, but maybe something else would also be detected by Webpack? I'm fine with the current changes anyway.

@vitaliy-guliy
Copy link
Contributor Author

That's why I used location.href instead.
But seems // @ts-expect-error (TS1343) helps to compile ignoring the error.

Did it work with location.href? I only used import.meta.url because I thought it is a pattern Webpack is looking for to do the proper transformations, but maybe something else would also be detected by Webpack? I'm fine with the current changes anyway.

It works with location.href, but I have to create an URL referring on compiled javascript, not the typescript file.

@svor svor mentioned this pull request Jul 22, 2021
34 tasks
@paul-marechal
Copy link
Member

Out of curiosity I tried the following:

this.worker = new Worker(new URL('./worker/worker-main', location.href));

It didn't work at all... So import.meta.url it is then!

@vitaliy-guliy vitaliy-guliy merged commit 4431303 into master Jul 26, 2021
@vitaliy-guliy vitaliy-guliy deleted the fix-frontend-plugins branch July 26, 2021 13:22
@github-actions github-actions bot added this to the 1.16.0 milestone Jul 26, 2021
dna2github pushed a commit to dna2fork/theia that referenced this pull request Aug 25, 2021
…r only) (eclipse-theia#9715)

* frontend-plugin: fix creating a webworker for frontend plugin

Signed-off-by: Vitaliy Gulyy <vgulyy@redhat.com>

Co-authored-by: Chris Wilson <titanicswimteam@hotmail.com>
Co-authored-by: Vincent Fugnitto <vincent.fugnitto@ericsson.com>
@thegecko thegecko mentioned this pull request Feb 13, 2022
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants