-
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
Support bundling Theia with esbuild #14414
base: master
Are you sure you want to change the base?
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.
Nice initiative! I'm on Ubuntu 22.04. Building is much faster 👍
I only did some quick test by starting the Browser and Electron apps. This is what I saw:
- The terminal does not start (browser and Electron app)
2024-11-08T15:54:15.608Z terminal ERROR TypeError: pty.fork is not a function
at new UnixTerminal2 (/theia/examples/browser/lib/backend/main.js:270250:26)
at spawn (/theia/examples/browser/lib/backend/main.js:270444:14)
at ShellProcess.createPseudoTerminal (/theia/examples/browser/lib/backend/main.js:270934:47)
at startTerminal (/theia/examples/browser/lib/backend/main.js:270900:25)
at new TerminalProcess (/theia/examples/browser/lib/backend/main.js:270921:43)
at new ShellProcess (/theia/examples/browser/lib/backend/main.js:271503:9)
at createInstanceWithInjections (/theia/examples/browser/lib/backend/main.js:25307:18)
at _createInstance (/theia/examples/browser/lib/backend/main.js:25298:16)
at resolveInstance (/theia/examples/browser/lib/backend/main.js:25390:16)
at _getResolvedFromBinding (/theia/examples/browser/lib/backend/main.js:25698:20)
2024-11-08T15:54:15.608Z root ERROR Error: pty process did not start correctly
at ShellProcess.checkTerminal (/theia/examples/browser/lib/backend/main.js:271012:17)
at get pid [as pid] (/theia/examples/browser/lib/backend/main.js:270965:14)
at /theia/examples/browser/lib/backend/main.js:271732:51
at /theia/examples/browser/lib/backend/main.js:31322:69
at CallbackList.invoke (/theia/examples/browser/lib/backend/main.js:31328:23)
at _Emitter.fire (/theia/examples/browser/lib/backend/main.js:31438:34)
at ShellProcess.emitOnError (/theia/examples/browser/lib/backend/main.js:269086:27)
at process.processTicksAndRejections (node:internal/process/task_queues:81:21)
- Starting Electron I get errors like these
TypeError: this._keymapping.onDidChangeKeyboardLayout is not a function
at NativeBinding.onDidChangeKeyboardLayout (/theia/examples/electron/lib/backend/electron-main.js:71800:26)
2024-11-08T16:10:17.975Z root ERROR TypeError: this._keymapping.getKeyMap is not a function
at NativeBinding.getKeyMap (/theia/examples/electron/lib/backend/main.js:129581:33)
2024-11-08T16:10:17.975Z root ERROR TypeError: this._keymapping.getCurrentKeyboardLayout is not a function
@sdirix Thank you for taking a look. I actually fell into a esbuild trap, that I didn't notice before. Using |
For some reason, the call to |
@msujew One question: would it be possible to switch to Vite? It includes esbuild and has some other benefits.. |
@nimo23 AFAIK Vite stopped using esbuild as part of their pipeline and completely switched to Rollup, see also their documentation. They say themselves that Vite is slower than esbuild for that reason. I know that Vite has a few benefits (and is easier to deal with in general), but Theia adopters don't need to configure too much anyway, as the cli already does most of the heavy lifting. Do you have a specific use case in mind that is difficult to implement using esbuild? |
Ok, good to know.
(1) Live Reload and Watch: According to esbuild docs (see https://esbuild.github.io/api/#live-reload):
In Vite, we can simply call
It would be nice to include a script run task in |
@nimo23 Unfortunately, it's not that easy. Webpack is already capable of doing hot reloading, but we're not making use of that either - mainly because we're serving the frontend from our own backend and not using a dev server for that. Whether we use Vite, webpack or esbuild doesn't matter for that - hot reloading is not really supported in Theia right now. I assume that a feature like that needs to be implemented into the backend/frontend of the app itself, and cannot be supplied by the bundler anyway. |
What it does
This PR adds support for bundling Theia with esbuild. Bundling Theia with webpack is still possible, however we might want to deprecate this in the future due to the maintenance effort if we go through with this PR.
This change essentially just adds three esbuild build contexts to the webpack-generator (now renamed bundler-generator) that are invoked via the command line.
I needed to do a few changes to our CSS, because esbuild didn't want to compile them. This mostly includes moving
~
based imports of other CSS files into code and moving the@import
statements to the top of the file. Additionally, esbuild doesn't want to bundle the CSS into the JS, but creates a separatebundle.css
file. This is now consumed via theindex.html
.The new bundler is way faster than webpack. I've observed a tenfold improvement, reducing the build time from 20 seconds to only 2 seconds.
How to test
Test everything. I.e. assert that every feature works in every configuration (browser, browser-only, electron). This specifically includes:
Follow-ups
As of now, this change is missing support for:
Review checklist
Reminder for reviewers