Skip to content
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

Merged
merged 7 commits into from
Sep 13, 2023

Conversation

msujew
Copy link
Member

@msujew msujew commented Jun 1, 2023

What it does

Currently, the browser preload scripts has 3 tasks:

  • Load the translations for the selected locale
  • Load the backend OS to correctly display stuff like line endings
  • Set the background of the application based on the last selected theme

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 called frontendPreload. This allows to define new PreloadContribution 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 via WebSocketConnectionProvider.

As a drive by, also:
Closes #12670
Closes #6675

How to test

  1. Confirm that the tasks outlined above still work as expected.
  2. Remove the frontendPreload from @theia/core/package.json. Assert that the preload module is removed from the generated index.js file.

Review checklist

Reminder for reviewers

@msujew msujew added the quality issues related to code and application quality label Jun 1, 2023
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.

Changes look mostly good to me but with a few comments.

@msujew
Copy link
Member Author

msujew commented Jun 28, 2023

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

@msujew
Copy link
Member Author

msujew commented Sep 11, 2023

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

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.

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

  1. git clean -ffdx
  2. checkout the branch
  3. yarn
  4. yarn electron build
  5. 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

@msujew
Copy link
Member Author

msujew commented Sep 12, 2023

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

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.

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 generated index.js file when frontendPreload is removed from the @theia/core/package.json

@vince-fugnitto vince-fugnitto added the extensibility issues to simplify ability to extend Theia label Sep 12, 2023
@msujew msujew merged commit 35865a9 into master Sep 13, 2023
13 checks passed
@msujew msujew deleted the msujew/preload-inversify branch September 13, 2023 22:09
@github-actions github-actions bot added this to the 1.42.0 milestone Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extensibility issues to simplify ability to extend Theia quality issues related to code and application quality
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Syntax issue with generated backend/frontend code [electron]: Generated code has errors
3 participants