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.

Sign up for GitHub

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

Jump to bottom

WIP: bundle main+preload as ES #15544

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

WIP: bundle main+preload as ES #15544

wants to merge 4 commits into from
+109 −74

Conversation

Lemonexe
Copy link
Contributor

@Lemonexe Lemonexe commented Nov 25, 2024 •
edited
Loading

Description

  • Bump electron-store to 10.0.0
    • ➡️ does no longer support CJS, build crashes
  • Webpack: change output target for main+preload bundles as ES instead of CJS
  • Electron-builder: configure to recognize ES
  • Fix optional externals deps, see comments below

Does not work 😞

  • Build is successful, the output is indeed ES.
  • However, upon starting the application, it freezes - no window is opened, but no error either.
  • It reads the app.js (main process bundle) but does not call the functions which initiate the browser window.
  • So there might be something wrong with the way webpack bundles ES
    • after all, it's an experimental feature

Thoughts? 💭

  • for main+preload, replace webpack with different bundler
    • but leave webpack for renderer
    • we used to have ESbuild, but replaced it with webpack because it supports chunking
    • explore https://rspack.dev/
    • vite or rollup supports chunking too, and has first-class support for ES
      • but could be a lot of work

Related Issue

Attempt to resolve #14482

Lemonexe
Lemonexe commented Nov 25, 2024
View reviewed changes
packages/suite-desktop-core/webpack/core.webpack.config.ts
// 'bufferutil', // optional dependency of ws lib
// 'memcpy', // optional depencency of bytebuffer lib
// 'utf-8-validate', // optional dependency of ws lib
// 'osx-temperature-sensor', // optional dependency of systeminformation lib
Copy link
Contributor Author

@Lemonexe Lemonexe Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

These are all sub-dependencies that are only optionally required by our dependencies, example.

When specified in externals, webpack will try to provide them, but it crashes at run time when output target is ES. I don't know if any actual suite functionality relies on those libs.

Lemonexe
Lemonexe commented Nov 25, 2024
View reviewed changes
packages/suite-desktop-core/webpack/core.webpack.config.ts
@@ -112,6 +113,8 @@ const config: webpack.Configuration = {
alias: {
'@emurgo/cardano-serialization-lib-nodejs': '@emurgo/cardano-serialization-lib-browser',
'@trezor/connect$': '@trezor/connect/src/index', // alternative for "module": "src/index" in connect's package.json
// optional dependency of systeminformation lib mocked as undefined (webpack needs it when targetting ES)
'osx-temperature-sensor': false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unlike other deps in externals, this one needs to be mocked as undefined, otherwise webpack crashes during building the desktop core @trezor/suite-desktop-core build:app:dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@komret komret Awaiting requested review from komret komret will be requested when the pull request is marked ready for review komret is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable support for ES modules
1 participant
@Lemonexe