Expose key helpers on the API for addons #5292
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,16 @@ | |
| * @license MIT | ||
| */ | ||
|
|
||
| import { Terminal, ITerminalAddon, IDisposable, EmitterCtorType, IEmitter, IEvent } from '@xterm/xterm'; | ||
| import type { ProgressAddon as IProgressApi, IProgressState } from '@xterm/addon-progress'; | ||
|
|
||
| // to use impl parts: | ||
|
|
||
| // in 3rd party addons | ||
| // import { EmitterAddon } from '@xterm/xterm'; | ||
|
|
||
| // in xtermjs repo addons | ||
| import { EmitterAddon } from 'shared/shared'; | ||
|
|
||
|
|
||
| const enum ProgressType { | ||
|
|
@@ -33,13 +40,18 @@ function toInt(s: string): number { | |
| } | ||
|
|
||
|
|
||
| export class ProgressAddon extends EmitterAddon implements ITerminalAddon, IProgressApi { | ||
| private _seqHandler: IDisposable | undefined; | ||
| private _st: ProgressType = ProgressType.REMOVE; | ||
| private _pr = 0; | ||
| private _onChange: IEmitter<IProgressState>; | ||
| public onChange: IEvent<IProgressState>; | ||
|
|
||
| constructor(protected readonly _emitterCtor: EmitterCtorType) { | ||
|
There was a problem hiding this comment. In retrospect it's obvious this would be a breaking change, but that's fine and worth it considering the wins we get in bundle size. There was a problem hiding this comment. About breaking change - as I wrote above, I wonder whether to make the xterm exports the first ctor argument for all addons, e.g.: import * as XtermApi from '@xterm/xterm';
import { AddonXY } from '@term/addon-xy';
const term = XtermApi.Terminal(...);
const addonXY = new AddonXY(XtermApi);Then an addon ctor is free to use the exported ctors there or to ignore that argument: import type * as XtermApi from '@xterm/xterm';
class AddonXY extends ... {
contructor(xterm: XtermApi, other_args) {
// if addon needs an event:
this._onWhatever = xterm.Emitter<Whatever>();
...
// else: just ignore xterm argument
}
}There was a problem hiding this comment. Or if thats too "globalish" looking, we could also aggregate the extra exports under a import { Terminal, shared } from '@xterm/xterm';
import { AddonXY } from '@term/addon-xy';
const term = new Terminal(...);
const addonXY = new AddonXY(shared);There was a problem hiding this comment. And last but not least - we could also keep import { Terminal } from '@xterm/xterm';
import { AddonXY } from '@term/addon-xy';
const term = new Terminal(...);
const addonXY = new AddonXY(Terminal.shared);There was a problem hiding this comment. The latter has a few advantages, like keeping stuff under the Terminal umbrella and automatically gaining access to those symbols even on a terminal instance. Edit: Tbh the ctor argument idea raises in fact the question, why not to load addons this way in the first place with a terminal instance as first argument. Do you remember, why we have the There was a problem hiding this comment.
Was a long time ago, but one of the big things we get is There was a problem hiding this comment. Yes it def. smells like quite the big API change would be needed, so idk exactly how to proceed. Maybe we should go back to conceptual structuring before inventing a square wheel here? Gonna try to do a write up of what we have currently vs. what could be done about it in #5283. There was a problem hiding this comment. Posted long replies in #5283 (comment) |
||
| super(_emitterCtor); | ||
| this._onChange = new this._emitterCtor<IProgressState>(); | ||
| this.onChange = this._onChange.event; | ||
| } | ||
|
|
||
| public dispose(): void { | ||
| this._seqHandler?.dispose(); | ||
|
|
@@ -81,9 +93,6 @@ export class ProgressAddon implements ITerminalAddon, IProgressApi { | |
| } | ||
| return true; | ||
| }); | ||
| } | ||
|
|
||
| public get progress(): IProgressState { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| /** | ||
| * Copyright (c) 2024 The xterm.js authors. All rights reserved. | ||
| * @license MIT | ||
| */ | ||
|
|
||
| import { IDisposable, IDisposableStore, DisposableStoreCtorType, EmitterCtorType } from '@xterm/xterm'; | ||
|
|
||
|
|
||
| export class DisposableAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
|
|
||
| constructor(protected readonly _storeCtor: DisposableStoreCtorType) { | ||
| this._store = new _storeCtor(); | ||
| } | ||
|
|
||
| public dispose(): void { | ||
| this._store.dispose(); | ||
| } | ||
| } | ||
Tyriar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
| export class EmitterAddon { | ||
| constructor( | ||
| protected readonly _emitterCtor: EmitterCtorType | ||
| ) {} | ||
| } | ||
|
|
||
|
|
||
| export class DisposableEmitterAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
|
|
||
| constructor( | ||
| protected readonly _storeCtor: DisposableStoreCtorType, | ||
| protected readonly _emitterCtor: EmitterCtorType | ||
| ) { | ||
| this._store = new _storeCtor(); | ||
| } | ||
|
|
||
| public dispose(): void { | ||
| this._store.dispose(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| { | ||
| "extends": "../tsconfig-library-base", | ||
| "compilerOptions": { | ||
| "lib": [ | ||
| "es2015", | ||
| "es2016.Array.Include" | ||
| ], | ||
| "outDir": "../../out", | ||
| "types": [ | ||
| "../../node_modules/@types/mocha" | ||
| ], | ||
| "baseUrl": "..", | ||
| "paths": { | ||
| "vs/*": [ "./vs/*" ] | ||
| } | ||
| }, | ||
| "include": [ | ||
| "./**/*", | ||
| "../../typings/xterm.d.ts" | ||
| ], | ||
| "references": [ | ||
| { "path": "../vs" } | ||
| ] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1956,4 +1956,73 @@ declare module '@xterm/xterm' { | |
| */ | ||
| readonly wraparoundMode: boolean; | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * Get Emitter constructor. | ||
| */ | ||
| export const emitterCtor: EmitterCtorType; | ||
|
|
||
| /** | ||
| * Get DisposableStore contructor. | ||
| */ | ||
| export const disposableStoreCtor: DisposableStoreCtorType; | ||
|
|
||
| /** | ||
| * Turn a function into a Disposable. | ||
| */ | ||
| export const toDisposable: (fn: () => void) => IDisposable; | ||
|
There was a problem hiding this comment. I think you should be able to There was a problem hiding this comment. Isn't that pulling types from internal sources into the public API? My idea here was to decouple that with minimal stub types, so linter / type inspection doesn't rely on internal stuff. There was a problem hiding this comment.
Everything in the API needs to be standalone, so we'd duplicate it there. Also, we depend the other way for Terminal here to ensure our implementation matches the API: xterm.js/src/browser/public/Terminal.ts Line 25 in d81b25c So we could do the same for |
||
|
|
||
|
|
||
| export interface IEmitter<T> { | ||
| dispose(): void; | ||
| event: IEvent<T>; | ||
| fire(event: T): void; | ||
| hasListeners(): boolean; | ||
| } | ||
|
|
||
| interface IDisposableStore extends IDisposable { | ||
| /** | ||
| * `true` if this object has been disposed of. | ||
| */ | ||
| isDisposed: boolean; | ||
| /** | ||
| * Dispose of all registered disposables but do not mark this object | ||
| * as disposed. | ||
| */ | ||
| clear(): void; | ||
| /** | ||
| * Add a new {@link IDisposable disposable} to the collection. | ||
| */ | ||
| add<T extends IDisposable>(o: T): T; | ||
| /** | ||
| * Deletes a disposable from store and disposes of it. | ||
| * This will not throw or warn and proceed to dispose the | ||
| * disposable even when the disposable is not part in the store. | ||
| */ | ||
| delete<T extends IDisposable>(o: T): void; | ||
| /** | ||
| * Deletes the value from the store, but does not dispose it. | ||
| */ | ||
| deleteAndLeak<T extends IDisposable>(o: T): void; | ||
| } | ||
|
|
||
| export type EmitterCtorType = new<T>() => IEmitter<T>; | ||
| export type DisposableStoreCtorType = new() => IDisposableStore; | ||
|
|
||
| export class DisposableAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
| constructor(_storeCtor: DisposableStoreCtorType); | ||
| public dispose(): void; | ||
| } | ||
| export class EmitterAddon { | ||
| protected readonly _emitterCtor: EmitterCtorType; | ||
| constructor(_emitterCtor: EmitterCtorType); | ||
| } | ||
| export class DisposableEmitterAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
| protected readonly _emitterCtor: EmitterCtorType; | ||
| constructor(_storeCtor: DisposableStoreCtorType, _emitterCtor: EmitterCtorType); | ||
| public dispose(): void; | ||
| } | ||
Tyriar marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's nicest to just pass in the whole xterm API object to some addons? I think we can do this without error:
That way it would be nicer from the embedder side:
Looks better than this imo:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thats a good idea. The dispoable + emitter ctors, if both are needed, already make this cumbersome and looking ugly. With the whole exports as one arguments it gets much nicer and easier to comprehend on caller side.
I even wonder if we should make that the first default argument on all addon ctors, this way ppl wont get it wrong on certain addons, just apply it on all (well thats a major API shift).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the full module export type interface stubbing felt kinda wrong. My next approach looks like that:
on addon impl side:
on embedding side: