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.

[Tyriar]

So maybe I've overlooked something crucial in that 2-stage construction setup, if so plz tell me.

If I can remember right the reason I went with 2 stage is so that the interface is very simple in xterm.d.ts:

xterm.js/typings/xterm.d.ts

Lines 1314 to 1322 in d81b25c

	/**
	* An addon that can provide additional functionality to the terminal.
	*/
	export interface ITerminalAddon extends IDisposable {
	/**
	* This is called when the addon is activated.
	*/
	activate(terminal: Terminal): void;
	}

Nothing dynamic here, just a simple activate call that is always the same. If we wanted to encode the Terminal part of the ctor call into the API it would end up being something like this:

constructor(terminal: Terminal, ...args: unknown);

Then we also need to either assume the addon is correctly implemented and actually called loadAddon as in your implementation:

  constructor(term: Terminal, ...someOtherNeededArgs: any[]) {
    ...
    term.loadAddon(this);

So we're leaving more up to addons doing the right thing.

While having things be optional in addons is a little nuisance sometimes, large addons can follow a pattern similar to WebglAddon where it's a small wrapped around WebglRenderer before it's activated:

xterm.js/addons/addon-webgl/src/WebglAddon.ts

Lines 18 to 20 in d81b25c

	export class WebglAddon extends Disposable implements ITerminalAddon , IWebglApi {
	private _terminal?: Terminal;
	private _renderer?: WebglRenderer;

Another benefit of this 2 stage approach that's obvious here is that we can initialize listeners of events before we activate the addon. Otherwise we would miss the events here if they fire during activate:

xterm.js/addons/addon-webgl/src/WebglAddon.ts

Lines 22 to 29 in d81b25c

	private readonly _onChangeTextureAtlas = this._register(new Emitter<HTMLCanvasElement>());
	public readonly onChangeTextureAtlas = this._onChangeTextureAtlas.event;
	private readonly _onAddTextureAtlasCanvas = this._register(new Emitter<HTMLCanvasElement>());
	public readonly onAddTextureAtlasCanvas = this._onAddTextureAtlasCanvas.event;
	private readonly _onRemoveTextureAtlasCanvas = this._register(new Emitter<HTMLCanvasElement>());
	public readonly onRemoveTextureAtlasCanvas = this._onRemoveTextureAtlasCanvas.event;
	private readonly _onContextLoss = this._register(new Emitter<void>());
	public readonly onContextLoss = this._onContextLoss.event;

I think WebglAddon.onContextLoss for example does fire synchronously after activate is called depending on hardware. Working around this would involve exposing the state of all events on the object itself (eg. .onContextLoss + .contextLossState) in order to correctly initialize and react to the initial state.

---

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

So given the above I think we must actually keep the 2 stage due to the event listening.

I actually wrote this up before reading through this issue and I think it's still relevant (your suggestion was kind of a hybrid of 1 and 2):

There are 4 main options I'm considering here:

1. Make a new v2 addon interface/loader. I'm not sure we can actually do this since we need the type when the addon is constructed, not when activate is called.
2. Move all Disposables in the addons to use composition instead of inheritance. This would maybe let us do things at activate time and therefore maybe inject the API into activate like this:

   activate(terminal: Terminal, xtermApi: ...): void;

   But it would add a code smell of properties being nullable before activate and we'd probably hit other problems too.
3. Just pass in the terminal API for first party addons as required in the ctor, don't try change core API.
4. Create a simple Disposable, Emitter, etc. similar to our old one in shared/.

I'm still leaning towards 3 which is a breaking change for addons, but we're already coming up on v6.0.0 anyway. The breaking change will be obvious when someone updates as the addon ctors will not compile.

---

Something else that comes to mind is maybe we want the addon to pull in a minimal set of things too. So instead of this:

constructor(xtermApi: XtermApi, opts...)

We do this:

constructor(disposableCtor: typeof DisposableStore, opts...)

Or this:

constructor(xtermApi: Pick<XtermApi, 'DisposableStore'>, opts...)

I'm trying to wrap my head around it but if we don't do this there's probably going to be a lot of people running into compile errors when the xterm.js version is even slightly out of sync with the addon version. I'm sure in practice these versions are never so strictly aligned (outside of VS Code where versions are always the most up to date beta).

[Tyriar]

To sum up, after reading everything and writing that last comment, I think these are the 3 options we have:

• (3 from before) Just pass in the terminal API for first party addons as required in the ctor, don't try change core API.
• (4 from before) Create a simple Disposable, Emitter, etc. similar to our old one in shared/.
• (new) Try simplifying the helper classes in question or split them out into new files to help with tree shaking. For example DisposableStore is relatively simple, but it uses setParentOfDisposable that calls into DisposableTracker that depends on arrays.ts

We can kind of just avoid all this if we bring back the old simple Disposable and Emitter and then add a MutableDisposable based on it (since i love that pattern) and whatever else we need to live in common/shared/.

(4 from before) Create a simple Disposable, Emitter, etc. similar to our old one in shared/

What do you think about just going this route and avoiding the breaking change all together?

[jerch]

Out of your list (3) would be my favorite currently, as it does the least "damage" to current API and addon setup. 3rd party addons not needing the goodies will just keep working as before. Still for the sake of uniformity I think we should apply the new argument to all first party addon ctors and just announce that as a major change of our addons.

About (4) - yes, that actually might be needed in conjunction with (3) in the long run, if the vs impls + types will shift alot. Exposing their ctors directly on the API kinda reintroduces that building-blocks-issue I ranted about above, but I see no reason, why we cannot hide bigger internal shifts by just wrapping things into more stable API bridging code and minimalistic types (similar to what we do for terminal internals).

My current PR indeed just forwards vs impls here https://github.com/xtermjs/xterm.js/blob/883483ba9ecbde2ad3997bef3871a943fbd41dfa/src/browser/public/Terminal.ts#L23-28, while I already tried to decouple the types by creating own minimalistic types in xterm.d.ts.

---

Edit:
Why I brought up the 2-stage construction issue - At a first glance it seems weird, to import things twice from xterm package into addons, once as module stuff and once as a terminal instance. I think that a single construction step would solve that and make the addon usage easier to grasp. But it is true, that this creates even more freedom in the addon ctor and might lead to accidentally misalign things. Also it def. would need a very big API change. Both is not the case for (3), which therefore seems preferable.