-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 inversify lifecycle to frontend preload script #12590
Conversation
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.
Changes look mostly good to me but with a few comments.
dev-packages/application-manager/src/generator/frontend-generator.ts
Outdated
Show resolved
Hide resolved
packages/core/src/browser/preload/theme-preload-contribution.ts
Outdated
Show resolved
Hide resolved
3aca353
to
29550b2
Compare
29550b2
to
62e8c49
Compare
@paul-marechal I've refactored the feature (again). The preload services are now integrated into the rpc messaging system. I believe this is the way to go forward with this PR. |
62e8c49
to
692ffa9
Compare
692ffa9
to
aa50408
Compare
@vince-fugnitto Any chance to look into this change? I think this could be useful for #12853, since the current preload script doesn't make a lot of sense for a CDN based Theia version. |
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.
@msujew the changes seem to work as expected for the browser use-case, however I'm experiencing errors attempting to start the electron application if you can confirm:
git clean -ffdx
- checkout the branch
yarn
yarn electron build
yarn electron start
- the following will fail
$ yarn electron start
yarn run v1.22.19
$ yarn -s --cwd examples/electron start
App threw an error during load
Error: Module parse failed: 'return' outside of function (47:4)
File was processed with these loaders:
* ../../node_modules/source-map-loader/dist/cjs.js
You may need an additional loader to handle the result of these loaders.
| // There is another instance running, exit now. The other instance will request focus.
| app.quit();
> return;
| }
|
at ./src-gen/backend/electron-main.js (/home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:10:7)
at /home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:21:68
at Object.<anonymous> (/home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:23:12)
at Module._compile (node:internal/modules/cjs/loader:1174:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1229:10)
at Module.load (node:internal/modules/cjs/loader:1044:32)
at Module._load (node:internal/modules/cjs/loader:885:12)
at f._load (node:electron/js2c/asar_bundle:2:13330)
at loadApplicationPackage (/home/foobar/workspaces/theia/node_modules/electron/dist/resources/default_app.asar/main.js:121:16)
at Object.<anonymous> (/home/foobar/workspaces/theia/node_modules/electron/dist/resources/default_app.asar/main.js:233:9)
A JavaScript error occurred in the main process
Uncaught Exception:
Error: Module parse failed: 'return' outside of function (47:4)
File was processed with these loaders:
* ../../node_modules/source-map-loader/dist/cjs.js
You may need an additional loader to handle the result of these loaders.
| // There is another instance running, exit now. The other instance will request focus.
| app.quit();
> return;
| }
|
at ./src-gen/backend/electron-main.js (/home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:10:7)
at /home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:21:68
at Object.<anonymous> (/home/foobar/workspaces/theia/examples/electron/lib/backend/electron-main.js:23:12)
at Module._compile (node:internal/modules/cjs/loader:1174:14)
at Module._extensions..js (node:internal/modules/cjs/loader:1229:10)
at Module.load (node:internal/modules/cjs/loader:1044:32)
at Module._load (node:internal/modules/cjs/loader:885:12)
at f._load (node:electron/js2c/asar_bundle:2:13330)
at loadApplicationPackage (/home/foobar/workspaces/theia/node_modules/electron/dist/resources/default_app.asar/main.js:121:16)
at Object.<anonymous> (/home/foobar/workspaces/theia/node_modules/electron/dist/resources/default_app.asar/main.js:233:9)
MESA-INTEL: warning: Performance support disabled, consider sysctl dev.i915.perf_stream_paranoid=0
libva error: /usr/lib/x86_64-linux-gnu/dri/iHD_drv_video.so init failed
libva error: /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so init failed
@vince-fugnitto Thanks for looking into it! Yeah, it seems like my changes completely messed up the generated electron-main.js file. It should work correctly now 👍 |
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.
LGTM 👍
- confirmed that the browser application builds and starts correctly
- confirmed that the electron application builds and starts correctly
- confirmed the
preload
module is removed from the generatedindex.js
file whenfrontendPreload
is removed from the@theia/core/package.json
What it does
Currently, the browser preload scripts has 3 tasks:
However, these tasks are hardcoded and adopters cannot add new scripts that are supposed to be executed during the preload phase of the frontend application. This change adds an inversify lifecycle to the aforementioned frontend preload scripts and a new contribution point to the
package.json
calledfrontendPreload
. This allows to define newPreloadContribution
items.It also changes the way communication within the preload script is being performed. Instead of using
fetch
, we use the normal rpc messaging system viaWebSocketConnectionProvider
.As a drive by, also:
Closes #12670
Closes #6675
How to test
frontendPreload
from@theia/core/package.json
. Assert that the preload module is removed from the generatedindex.js
file.Review checklist
Reminder for reviewers