xtermjs / xterm.js

A terminal for the web
https://xtermjs.org/
MIT License
16.99k stars 1.59k forks source link

Include base/ platform from VS Code and leverage DomScrollableElement #5076

Closed Tyriar closed 5 days ago

Tyriar commented 1 week ago

This is a work in progress that leverages DomScrollableElement from VS Code instead of our very fragile and hacky Viewport. This is something I've wanted to do for a long time but wasn't sure of how to accomplish it. This gives us a bunch of wins:

Performance is mostly on par with what we had previously.

I haven't looked into the resulting bundle yet but my thinking is that this will worth it as this is not the only thing we can leverage (eg. various widgets, helpers, data structures, etc.) and we could also remove some of our own utils that are pulled from VS Code's codebase (like Lifecycle.ts, potentially even InstantiationService). Only the imported files should be included in the bundle, I'm not sure what the tree shaking situation is though. I've optimized the vs/base inclusion for easy tracking of the vscode main branch for when new useful changes come in.

Here's what it looks like atm:

Recording 2024-06-28 at 13 43 46

When the overview ruler is enabled a black border now shows up and it's pixel perfect with the scroll bar:

image


TODOs in src/browser/ and src/common/ the code track most remaining items. Here are the high level items:

@jerch I'm curious on your thoughts on this change

jerch commented 6 days ago

@Tyriar Oh wow this looks promising. But +234,923 −438 changes? Is that for real or does that still need a basic cleanup? I'll def. need some time to go through this :fearful:

Tyriar commented 6 days ago

@jerch I might be able to exclude some of the folders (eg. src/vs/base/**/test/*), one of the main things I wanted to accomplish here was to just copy and paste the entirety of the folder with minimal changes to make it really easy to bring in any upstream changes. I think the only change that is required right now is to commend out the css includes (import 'vs/css!...';). A git submodule is also an option if we wanted to keep the codebase a little more clean at the cost of a little extra overhead in updating it.

To "review" this I wouldn't go through all of base/, but look at:

Tyriar commented 6 days ago

Removed a bunch of files in the latest, still +82k but a lot of this is code that we would be able to leverage now

Tyriar commented 6 days ago

Old

Warnings for yarn package:

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  xterm.js (285 KiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (285 KiB)
      xterm.js

WARNING in webpack performance recommendations: 
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

New

Warnings for yarn package:

WARNING in ./out/base/common/network.js 222:49-56
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ ./out/base/browser/dom.js
 @ ./out/base/browser/ui/scrollbar/scrollableElement.js
 @ ./out/browser/Viewport.js 17:28-85
 @ ./out/browser/Terminal.js 35:19-46
 @ ./out/browser/public/Terminal.js 5:19-46

WARNING in ./out/base/common/network.js 263:49-56
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ ./out/base/browser/dom.js
 @ ./out/base/browser/ui/scrollbar/scrollableElement.js
 @ ./out/browser/Viewport.js 17:28-85
 @ ./out/browser/Terminal.js 35:19-46
 @ ./out/browser/public/Terminal.js 5:19-46

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets: 
  xterm.js (645 KiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
  main (645 KiB)
      xterm.js

WARNING in webpack performance recommendations: 
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/
Tyriar commented 6 days ago

Looked into esbuild again today as I wasn't happy with the tree shaking in webpack. Looks like it's not good enough to find some important unused parts either, so I'm thinking maybe I need to manually include files which will increase the update from upstream burden but keep the bundle size as low as possible.

jerch commented 5 days ago

@Tyriar Thx for the cleanup, looks much more digestible now :smile_cat:

About webpack and its tree shaking - I am not very deep into this, but it somehow rings a bell. If I remember right, their dead code elimination is heavily based on ESM module boundaries & exports, which by nature wont work well with non ESM-based projects. Not sure though if they fixed that with more recent releases...

Tyriar commented 5 days ago

Yeah I tried switching to ESM and it still doesn't eliminate some particularly troublesome functions like this that are definitely not used, esbuild is the same. I'll do it manually today but keep the update script so it's still relatively easy to pull in changes (run script, then delete a bunch of files and maybe functions until the diff looks good).

Tyriar commented 5 days ago

Continuing on esbuild branch: https://github.com/xtermjs/xterm.js/pull/5077