Expose key helpers on the API for addons #5292
Conversation
|
@Tyriar Ah well, still not there - tried to simplify the imports/exports, which again led to pulling vs stuff into addon 😊 Need to fix that again... |
|
Well treeshaking is not working with webpack, a 3rd party addon would still include the whole xterm.js lib. Needs a separate shared.js output in the final xterm.js (not gonna fix that today...) |
I wouldn't worry about 3rd party libs using our helper. They will have access to Emitter and Event via the API so they can make the tiny wrappers on their end if they need them. Not worth it imo. |
| @@ -35,6 +35,7 @@ const config = { | |||
| common: path.resolve('./out/common'), | |||
| browser: path.resolve('./out/browser'), | |||
| vs: path.resolve('./out/vs'), | |||
| shared: path.resolve('./out/shared') | |||
There was a problem hiding this comment.
By shared/ I meant inside common somewhere, similar to browser/renderer/shared/'s usage in webgl
| /** | ||
| * 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 export class here, similar to export class Terminal. That way we don't eed to deal with this CtorType stuff
| export class DisposableEmitterAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
| protected readonly _emitterCtor: EmitterCtorType; | ||
| constructor(_storeCtor: DisposableStoreCtorType, _emitterCtor: EmitterCtorType); | ||
| public dispose(): void; | ||
| } |
There was a problem hiding this comment.
I wouldn't have thought this would be necessary? Emitters are generally several props on the object, not part of the base class.
| export class DisposableAddon implements IDisposable { | ||
| protected readonly _store: IDisposableStore; | ||
|
|
||
| constructor(protected readonly _storeCtor: DisposableStoreCtorType) { | ||
| this._store = new _storeCtor(); | ||
| } | ||
|
|
||
| public dispose(): void { | ||
| this._store.dispose(); | ||
| } | ||
| } |
There was a problem hiding this comment.
This is all this file needs to include imo. Though I'd call it AddonDisposable, as in a disposable for use in addons. DisposableAddon signals to me that it's only intended for extending on the main addon object, not other parts of the addon.
| private _onChange: IEmitter<IProgressState>; | ||
| public onChange: IEvent<IProgressState>; | ||
|
|
||
| constructor(protected readonly _emitterCtor: EmitterCtorType) { |
There was a problem hiding this comment.
Maybe it's nicest to just pass in the whole xterm API object to some addons? I think we can do this without error:
import type * as XtermApi from '@xterm/xterm';
import type { Terminal, ITerminalAddon, IDisposable } from '@xterm/xterm';That way it would be nicer from the embedder side:
new ProgressAddon(xterm)
new WebglAddon(xterm)
etc.Looks better than this imo:
new ProgressAddon(AddonDisposable)
new WebglAddon(AddonDisposable)| 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.
Attempt to fix #5283.
@Tyriar
The addon stubs under
/sharedonly add ~500 bytes on the xtermjs package, and also on addons that derive their addon class from it. Only inconvenience so far is the fact, how we synthesize the final xtermjs package, which leads to an import switch:import { ... } from 'shared/shared';Currently I have only one testbed implemented in
ProgressAddon.tsusing theEmitterAddonclass. Should be the same withDisposableAddon, but I didnt want to change too much in code before getting some feedback from you. So plz have a look at the idea and whether it goes into the right direction. Also we kinda need to cut type ropes for the vs objects, so plz also see the added types in d.ts - we prolly should keep them as small as possible while still being useful.And last but not least the question, whether we should put these additions under a separate name in the API (maybe
shared.*?)