[go: up one dir, main page]

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

Support bundling Theia with esbuild #14414

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

msujew
Copy link
Member
@msujew msujew commented Nov 7, 2024

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 separate bundle.css file. This is now consumed via the index.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:

  • plugin support
  • webview support
  • notebook support
  • search-in-workspace support
  • terminal support
  • build/watch modes

Follow-ups

As of now, this change is missing support for:

  • the export loader used for API testing (?)
  • compressed artifacts

Review checklist

Reminder for reviewers

@msujew msujew added the performance issues related to performance label Nov 7, 2024
Copy link
Member
@sdirix sdirix left a 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

@msujew
Copy link
Member Author
msujew commented Nov 9, 2024

@sdirix Thank you for taking a look. I actually fell into a esbuild trap, that I didn't notice before. Using '.node': 'file' doesn't actually load the file - it just replaces the import with a file path (which is pretty useless, to be honest). Anyway, I performed some changes to the esbuild plugin and the .node files should be loaded correctly now.

@msujew
Copy link
Member Author
msujew commented Nov 9, 2024

For some reason, the call to rg seems to fail during the runtime (it finds the rg/rg.exe file, but the calls result in error code 1). I'll not entirely sure why. I'll take a look at at that during the week.

@nimo23
Copy link
nimo23 commented Nov 10, 2024

@msujew One question: would it be possible to switch to Vite? It includes esbuild and has some other benefits..

@msujew
Copy link
Member Author
msujew commented Nov 10, 2024

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

@nimo23
Copy link
nimo23 commented Nov 10, 2024

AFAIK Vite stopped using esbuild as part of their pipeline and completely switched to Rollu

Ok, good to know.

Do you have a specific use case in mind that is difficult to implement using esbuild?

(1) Live Reload and Watch:

According to esbuild docs (see https://esbuild.github.io/api/#live-reload):

There is no esbuild API for live reloading directly. Instead, you can construct live reloading by combining watch mode (to automatically start a build when you edit and save a file) and serve mode (to serve the latest build, but block until it's done) plus a small bit of client-side JavaScript code that you add to your app only during development.

In Vite, we can simply call npm run dev to enable both Live Reload and Watch while developing:

// package.json
"scripts": {
    "dev": "vite",
    "build": "tsc -b && vite build",
    "preview": "vite preview"
},

It would be nice to include a script run task in package.json that also combines Live Reload and Watch with esbuild.

@msujew
Copy link
Member Author
msujew commented Nov 10, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance issues related to performance
Projects
Status: Waiting on reviewers
Development

Successfully merging this pull request may close these issues.

3 participants