Expose important pieces of vs folder on API

[jerch]

Coming from #5251 (comment)

Currently using Event, Disposable etc. from the vs/* path in addons adds ~50kB on addons. We should investigate, if and how we can save some binary size on addons by exposing crucial parts on the API, so the impls dont have to be copied over for every single addon.

[Tyriar]

Good idea for common things like Event👍

[jerch]

Looking through the list of addons, these would benefit from exposing event/emitter stuff on the API:

• image addon
• search addon
• webgl addon

[jerch]

@Tyriar Did some further digging around Event/Emitter and Disposable:

• Event/Emitter:
  • pulls ~40kB into an addon
  • easy to APIfy, since its usage narrows down to a new Emitter<T>() call
  • call should be moved from ctor to activate, where the addon gets a hold of the terminal object
• Disposable:
  • pulls ~25kB into an addon
  • hard to APIfy, as it is an abstract base class meant to be used with class extends Disposable
  • Disposable methods & ctor have a relative low footprint and only call into statics in vs/base/common/lifecycles (no further inheritance chaining)
  • --> might be possible to provide the same logic as a ctor mixin via API to be constructed at runtime (not sure yet...)
• Disposable + Event/Emitter:
  • pulls ~50kB into an addon (Disposable + Emitter share a lot of code)

Given the list of addons using vs/* imports those are the potential savings on the minified bundles:

• image addon: uses Disposable, APIfied would save ~25kB (current ~80kB)
• search addon: uses Emitter, APIfied would save ~40kB (current ~50kB)
• webgl addon: uses Disposable + Emitter, APIfied would save ~50kB (current ~240kB)

[Tyriar]

@jerch if you don't extend the typical thing is to use DisposableStore as a property instead. That's less convenient sometimes but if it'll save a bunch we can use that approach exclusively in addons instead of extends? Example:

https://github.com/microsoft/vscode/blob/decaa08e9945fad06912b5aafbd8f0f0e88e11c2/src/vs/workbench/contrib/terminal/browser/terminalView.ts#L69

Did you want to take a stab at a PR for this or want me to, since I introduced the problem? 😉

[jerch]

@jerch if you don't extend the typical thing is to use DisposableStore as a property instead.

Oh ic, yeah that might be the easier pattern then. Although I found a pattern with runtime extends leading to the same inheritance chain, schematically:

/////////////////
// before
// in AddonXY.ts
import { Disposable } from 'vs/base/common/lifecycle';
export class AddonXY extends Disposable { ... }

/////////////////
// with runtime extends
// in core terminal (quickly hacked into core)
  private helperCls = class extends Disposable {}
  public disposableCtor(): {new(): Disposable} {
    return this.helperCls;
  }

// in AddonXY.ts
import type { Disposable } from 'vs/base/common/lifecycle';
export function AddonXY(terminal: Terminal, ...) {
  const DisposableBase = (terminal as any)._core.disposableCtor() as {new(): Disposable};
  const AddonXY = class extends DisposableBase { ... };
  return new AddonXY(...);
}

It works basically the same as before, but now needs a terminal arg at the fake ctor already to derive the Disposable impl from there. Its not nicely abstracted yet, the helperCls looks like a code smell but is needed as the base class is abstract. Beside that it fully maintains the correct inheritance chain with a working super(). It is somewhat a class mixin pattern, but also relies on ES5 new behavior. Maybe the class factory aspect could be stated more clearly by separating the concerns, whatever if preferrable.

Did you want to take a stab at a PR for this or want me to, since I introduced the problem? 😉

Up to you. I can try to formalize that idea, though I am not 100% sure yet, if this works properly in all circumstances. At the moment this is a full drop-in replacement with only the terminal argument as difference.

Edit: To get instanceof properly working the class factory part should be separated from the instantiation for sure.

[Tyriar]

Runtime extends looks pretty confusing to me. The composition approach isn't quite as convenient but it's super clear how it works. We could also have a special AddonDisposable helper in shared/ that just wraps the API object?

// shared/
export class AddonDisposable {
  constructor(storeCtor: DisposableStoreCtor) {
    // init protected readonly _store
  }
  dispose() {
    this._store.dispose();
  }
}

// addon
export class WebglAddon extends AddonDisposable {
  constructor() {
    super(xterm.DisposableStore);
  }
}

That way only the class in shared/ will be pulled into the addon bundle

[jerch]

Yepp, +1 to composition. It also needs less awkward APi additions.

[jerch]

@Tyriar Would we need a separate xterm-shared for external addon creators, or will installing the xterm package with extended API be enough in the end to build an external addon? To me it seems quite involved if the full xterm source would be needed just to build a tiny addon xy.

[Tyriar]

@jerch I don't think so, it'll be a very small wrapper and they'll only be gaining things by having new stuff exposed on the API.

[jerch]

It is kinda obvious, that things here might lead to a bigger API change. So its prolly better to discuss a few things to not get on the wrong track.

current situation

xterm base package:

• ~340 kB in size
• monolithic build
• strong API shield
• more recently: contains lots of internal goodies from vs source

addons (plugins):

• either build from base repo (possibility to see and use internal goodies) or independently by a 3rd party
• various sizes depending on functionality (typically between 5kB - 50kB)
• monolithic builds for addons from base repo
• source: no harsh coding restrictions (in fact anything can be done in addons), to properly interact with a term instance it has only to follow the dispose pattern and to implement the activate method (works like a 2nd initializer with terminal instance as argument)
• lifecycle: spawned/instantiated independently, terminal will take dispose-responsibility on loadAddon
• more complex addons highly benefit from core goodies, but bloat the package sizes due to monolithic bundling

non-monolithic / non-single-API bundling for the rescue?
While that would solve the package size issues, it comes to a rather high price - it will only work, if we'd reshape the xterm package as a building-blocks-lib to be used by embedders along with addons directly accessing those internal building blocks. The downside is quite obvious - we would lose API stability guarantees, as internal changes would surface frequently. Thats not a good idea for a lib that widely used. Tbh fast turning APIs is what I hate most in the npm/JS ecosystem, it can turn any bigger project with lots of package deps into a maintenance/upgrade nightmare. We should not do the same mistake. So yes, API stability is a good thing, we should stick to that principle, even for the price of slightly bigger packages. That also means, that I am not gonna question an addon/plugin system at all, as it is a crucial part of the offered core API stability while still being extendable.

Whats the actual issue?
More complex addons benefit alot from goodies already linked and used in xterm core. They'd better make use of them to be in line with xterm core and for better overall code quality, before coming up with their own wonky implementation. But those core goodies have a quite high code footprint, which makes them bloat addon package size by a great amount (see #5292 (comment)). For a browser-based terminal integration thats a resource nightmare caused by nonsense doubled/tripled code.

ideas for a solution
Almost all goodies can be used with composition pattern at runtime. In fact they are also mostly used that way in core itself (like Event/Emitter and several lifecycle/disposal structures), we just would need to find a way to pass their implementation (function pointers) to addons at runtime. Addons itself are also loosely bound to xterm by composition, so thats kinda a natural fit here.

Current addon invocation and relation/lifecycle relative to xterm:
Since xterm v4 addons are class based, thus allow a much wider OOP-ish state handling and data/code encapsulation than the single function approach before (Sidenote: Im pretty aware that the same can be done with function-style coding nowadays in JS, complicated handcrafted closures is still OOP in the end and not the point here.) Addons also would expose additional functionality on their own (e.g. fitAddon.fit() in v4 vs. terminal.fit() in v3). For this great independence to work while still be able to interact with the terminal there is a contract in place between xterm and addons:

• addon has to implement an activate(term: Terminal) method being called from xterm side (for most addons this is used a second initializer or even ctor)
• access to xterm state should only be done via the terminal instance given to that method
• for addon's own stability sake - access should only be done via xterm's official API
• addon must implement a dispose() method (also being called from xterm for cleanup)

Beside that there are no further restrictions. The last point is an important aspect of the addon instance's lifecycling:

const xy = new AddonXY(customArgs);
/*  Now instance xy is alive but not yet attached to a terminal instance.
    For most addons this means, it cannot do anything yet
    (most addon have no explicit or only tiny stub ctors).
 */
term.loadAddon(xy);
/*  Thats the crucial call for the addon registering it to a terminal instance.
    `loadAddon` does some bookkeeping of the instance and
    takes the ownership for final disposal (in case the whole terminal
    got disposed, so ppl dont have to cleanup addon instances themselves)
    Finally `loadAddon` will call into `activate`...
*/
--> xy.activate(term);
/*  The second or true initializer of the addon is finally called with
    the terminal instance as argument. Most addons will postpone their real init code
    to that method, as they have no means for any action without a terminal instance.
*/
...
// later on addon disposal happens either by:
xy.dispose() // or
term.dispose()

So we basically have a 2-stage construction pattern for addons with the ctor doing its memory object creation thing and activate doing the post terminal instance registering setup.

But why a 2-stage construction?
Thats actually not clear to me. Was it for historical reasons derived from the older function currying-approach? Back to the issue at hand this 2-stage construction creates a rather unpleasant friction - there are goodies in xterm lib, that can easily be exposed to addons by composition, but are needed at ctor time. Which is currently not possible, as the addon does not get a hold of the xterm stuff before the 2nd init step in activate and its terminal instance.

So maybe I've overlooked something crucial in that 2-stage construction setup, if so plz tell me. The only thing I can imagine is a strong need of independence between stage 1 and stage 2 (between ctor and activate call), e.g. for doing async stuff in between (note ctors in JS are strictly sync itself). At least within our current addons we have none which would suffer from such a restriction.

Proposal:
Reshape addon API/contract to a 1-stage construction pattern with the terminal instance as mandatory first argument:

class AddonXY implements ... {
  constructor(term: Terminal, ...someOtherNeededArgs: any[]) {
    // pre-registering setup goes here (prev code in ctor)
    onChange = new term.shared.Emitter<Wotever>();
    ...
    term.loadAddon(this);
    // post-registering setup goes here (prev code in activate)
    ...
  }
}

This might need some different handling at taking the disposal ownership in the addon manager, but should be doable. The real benefit of such a simplified addon construction is the fact, that we can expose the needed goodies on the terminal instance itself, so the addon ctor can use them normally without getting the IEmitter | undefined nightmare.

Finally I came back to the issue at hand, ewww. It is much much easier to solve with a single step addon construction than it is now with the ctor/activate separation. If we stick to the composition pattern and reduce the addon construction to a single step we should be able to avoid most of the doubled/tripled code issues from addons.

---

@Tyriar Sorry for the wall of text, I admit that I got lost myself a bit in the middle weighing the pros/cons. I still think the big arc was needed to transport the whole picture.