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

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

Closed
wants to merge 16 commits into from

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Jun 28, 2024

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:

  • Action TODOs
  • Make sure all options work as expected
  • Sort out CSS variable situation
  • Investigate bundle size difference and its impact on startup time
  • Investigate what's included in the bundle - is tree shaking possible?
  • Investigate what files are included in base/, should we be git excluding some parts?
  • Polish the new tsconfig.json
  • Ensure licensing is all good (marked, dompurify, vscode)

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

@Tyriar Tyriar self-assigned this Jun 28, 2024
@jerch
Copy link
Member

jerch commented Jul 1, 2024

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

@Tyriar
Copy link
Member Author

Tyriar commented Jul 1, 2024

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

  • All the changes outside of src/vs
  • What we import:
    import { DomScrollableElement } from 'vs/base/browser/ui/scrollbar/scrollableElement';
    import type { ScrollableElementChangeOptions } from 'vs/base/browser/ui/scrollbar/scrollableElementOptions';
    import { ScrollbarVisibility, type ScrollEvent } from 'vs/base/common/scrollable';
  • What we will likely import like:
    • base/common/event.ts which contains some very handy helpers like Event.any, Event.debounce, Event.accumulate, PauseableEmitter. We could maybe remove common/EventEmitter.ts in favor of this.
    • base/common/lifecycle.ts which has DisposableTracker for detecting leaks, DisposableMap, etc. We could maybe remove common/Lifecycle.ts in favor of this.
    • base/browser/ui containing various widgets that might be useful, eg. sash
    • base/common/async lots of helpers around dealing with async/promises.

@Tyriar
Copy link
Member Author

Tyriar commented Jul 1, 2024

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
Copy link
Member Author

Tyriar commented Jul 1, 2024

Old

  • Demo (dev mode) 5520KB
  • yarn package:
    • xterm.js 285KB
    • xterm.js.map 1138KB
  • yarn package-headless:
    • xterm-headless.js 139KB
    • xterm-headless.js.map 619KB

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

  • Demo (dev mode) 7932KB ⬆️
  • yarn package:
    • xterm.js 646KB ⬆️
    • xterm.js.map 2510KB ⬆️
  • yarn pakcage-headless:
    • xterm-headless.js 139KB
    • xterm-headless.js.map 619KB

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
Copy link
Member Author

Tyriar commented Jul 1, 2024

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
Copy link
Member

jerch commented Jul 2, 2024

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

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
Copy link
Member Author

Tyriar commented Jul 2, 2024

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
Copy link
Member Author

Tyriar commented Jul 2, 2024

Continuing on esbuild branch: #5077

@Tyriar Tyriar closed this Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants