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

Custom addons #1128

Closed
feamsr00 opened this issue Nov 16, 2017 · 39 comments · Fixed by #2065
Closed

Custom addons #1128

feamsr00 opened this issue Nov 16, 2017 · 39 comments · Fixed by #2065
Assignees
Labels
area/api type/enhancement Features or improvements to existing features
Milestone

Comments

@feamsr00
Copy link

Hello all!

I'm trying to create a new addon and the documentation doesn't seem to be terribly clear on what it takes to go from 0-60.

For example, 1) exactly how does the API for add-ons differ from the nominal Terminal API? Does it at all? Will it?* Additionally, 2) is directly modifying Terminals' prototype the most appropriate way for add-ons to register functionality? It seems to be just asking for collisions. Is there any other namespace registration or dic facility? (I suppose maybe even just adding the addon object eg Terminal.MyAddon.method() for the simplest way, but surely Terminal.addon('MyAddon').method() is much more sound). 3) Also, it doesn't seem to be clear how to actually add a third party addon, as the names have all been hardcoded... (I have taken to extending Terminal and widening loadAddon (static loadAddon(String): void;)) I am making assumptions, since the docs don't say if loadAddon is only for private (non 3rd party) use.

I have reviewed some of the existing add-ons, but they don't seem to have the most consistent implementations.

*I'm creating a couple add-ons (at least one for custom escape codes) because I wish to add discrete functionality to xTerm.js. It seems far more architecturally sound to do that instead of just making a big abstraction layer on top of xTerm.

Details

  • tsc.exe version: 2.6.1
  • xterm.js version: 3
@jerch
Copy link
Member

jerch commented Nov 17, 2017

somewhat offtopic: For custom escape codes there is imho no way to hook into the default parser atm. You can still get those custom codes working with your own parser beforehand. The ANSI spec allows custom escape codes for OSC and DCS commands. Unless you know what you are doing - stick with those to stay compatible with the default parser.

@parisk parisk self-assigned this Nov 17, 2017
@parisk parisk added the type/documentation Regarding website, API documentation, README, etc. label Nov 17, 2017
@parisk parisk added this to the 3.0.0 milestone Nov 17, 2017
@parisk
Copy link
Contributor

parisk commented Nov 17, 2017

Hello @feamsr00; good points.

  1. Add-ons should be using the public API of xterm.js only to provide additional functionality that does not fit in the core (e.g. attaching on a websocket)
  2. Directly modifying the prototype is not considered a good practice, but it has worked out well until now. It provides a simpler interface than using an add-on registry. Also none of the add-ons in the repository clashes with any of the rest, so this is not a problem till now.
  3. Indeed it's not documented how to create an add-on, but we can fix this in 3.0.
  4. ⚠️ Because of Master issue for webpack/loadAddon issues #1018 we will be ditching loadAddon in favor of a new cleaner API. Hopefully I will open a PR today 😄

@feamsr00
Copy link
Author

@jerch Thanks for the insight. I was worried about that. This kinda goes back to the API question. Even without a special API for add-ons per se, It's not clear how one could merely call a "Terminal.setParser()" (especially considering no such method seems to exist) to set that dependency. Moreover, it would be helpful if there was some interface contract for Parser. Are there any custom parsers in the wild, or other examples on how to move forward?
Might I suggest a feature to at least emit parser/ctrl events?

@Tyriar
Copy link
Member

Tyriar commented Nov 18, 2017

@feamsr00 currently there are no real external addons, everything is built in. The main reason we have not actively encouraged the community building addons right now is all they would be able to do is call the standard API of which you can't really do much interesting stuff that would call for an addon.

An addon could of course call into private members but it will likely break in the future as we don't commit to keeping private APIs stable. The addons packaged with the repo have a better chance of being stable at any given time.

It seems to be just asking for collisions. Is there any other namespace registration or dic facility?

100%, I really want to isolate the addons in a better and more consistent way.

Indeed it's not documented how to create an add-on, but we can fix this in 3.0.

I'd prefer we didn't rush in to this but instead fully think out #808 and make that the standard way to build an addon. We're also mid way through the discussions on what the future of loadAddon is, best let that bake for a while to see if there are any bumps in the road.

Are there any custom parsers in the wild, or other examples on how to move forward?

Swapping out renderers is something we've talked about before, not parsers though as the parser is quite complex. I think there are plans to allow extending of the parser with #576


@feamsr00 as you actually have some things you want to build, your feedback would be invaluable in #808. The hope here was to move a bunch of built in functionality to use this nice and modular component API and then make it available for use externally in addons.

@Tyriar
Copy link
Member

Tyriar commented Dec 11, 2017

Let's defer this so we can get v3 out

@feamsr00
Copy link
Author

And so, v3 is out now (🎉). What are the plans for the addon system?

@Tyriar
Copy link
Member

Tyriar commented Apr 11, 2018

No update, we still want to do it eventually though. I'm particularly keen on separating the built-in addons from the core repo.

@Tyriar
Copy link
Member

Tyriar commented Jun 3, 2018

Find below a proposal for proper addons that I wrote up while in the air ✈️ 😄. Any feedback would be greatly appreciated as if go ahead and make mistakes they'll be hard to change.

/cc @xtermjs/core @jerch @vincentwoo @chabou @amejia1 @jluk

Better Addons Proposal

xterm.js addons are components that use the xterm.js API to provide additional functionality. They follow a particular structure to make developing an addon convenient.

The difference between writing functionality in an addon and just using the API is that the Terminal is aware of addons and can provides additional hooks/functionality that you don't normally get when just programming against the API. It's also easy to develop and share your work in a consistent way with the community.

While it's certainly possible to write addons in JavaScript, TypeScript is encouraged due to the additional compiler type checks and first-class TS support in the core library.

Versioning

Since xterm.js is a living project and breakages do occur (on major versions), there's a chance an addon could break.

Loading addons

Instead of what was done previously, registering addons in the static Terminal.applyAddon function, addons are now passed into Terminal in the constructor as an optional 2nd argument:

interface ITerminalAddonConstructor<T> {
  // Used to make sure the same addon isn't instantiated twice
  readonly NAME: string;
  new(terminal: number): T;
}
class Terminal {
    constructor(
        options: ITerminalOptions,
        addons?: ITerminalAddonConstructor[]
    )
}

This allows different terminals to have a different set of addons and provides a convenient way to load the addons for each of them. Also note that the typeof an ITerminalAddon is provided

import { Addon1 } from '...';
import { Addon2 } from '...';

const addons = [Addon1, Addon2];

const terminal = new Terminal({}, addons);

xterm-base npm module

This module contains declaration files that define the interface of an addon. The only reason this module exists is so addons do not need to depend on the xterm module (and create a circular dependency).

                 xterm.d.ts
                      ^
                      |
             -------------------
             ^        ^        ^
             |        |        |
           xterm  xterm-fit   ...

This can be published to npm in a similar way that the vscode API is published, the source of truth remains in the xterm.js repo (and is referenced directly by xterm.js repo), but is published as a separate module for addons to consume.

Interfaces

Addons define a constructor which is triggered during Terminal's constructor, other hooks should be added using Terminal.on.

interface ITerminalAddon {
    /**
     * The minimum version of xterm.js that this addon is compatible with, in the format
     * `[major, minor, patch]`. if this is higher than the version being used it will throw an
     * `Error` when the addon is loaded.
     */
    minimumVersion: [number, number, number];

    /**
     * The maximum version of xterm.js that this addon is compatible with, in the format
     * `[major, minor, patch]`. If this is defined and lower than the version being used it will
     * throw an `Error` when the addon is loaded.
     * TODO: Should we bother with this? Are people going to bother updating the addon to add this?
     */
    maximumVersion?: [number, number, number];

    // This can be a starting point, with more hooks added later
    constructor(terminal: ITerminal);

    // Terminal will call this when Terminal.dispose is called, this can clean up any
    // references/elements it needs to to shut down gracefully and eventually be
    // used to turn addons off without disposing the terminal
    dispose(): void;

    // We could add more hooks here if we want, or just let addons listen in on internals using
    // `Terminal.on` (which wouldn't be as nicely typed). See xtermjs/xterm.js#808
}

To actually call functions on the addon you need to acquire the addon instance of a terminal. This can be done by leveraging the relatively complex Terminal.getAddon which takes an addon constructor and returns the addon instance. The internals of getAddon should be easy to implement:

interface ITerminalAddonConstructor<T> {
  new(terminal: number): T;
}
interface ITerminalAddon {
  a(): void;
}
class SearchAddon implements ITerminalAddon {
  findNext(): void {}
  a(): void {}
}
class FitAddon implements ITerminalAddon {
  fit(): void {}
  a(): void {}
}
class Terminal {
  getAddon<T extends ITerminalAddon>(addonConstructor: ITerminalAddonConstructor<T>): T {
    // Search internal addons list for that which matches addonConstructor
    return <T><any>1;
  }
}
const term = new Terminal();
const search = term.getAddon(SearchAddon);
search.findNext(); // Strongly typed simply by using the ctor
const fit = term.getAddon(FitAddon);
fit.fit(); // Strongly typed simply by using the ctor
term.getAddon({}); // error

The current "bundled" addons

Bundled addons will each move to the following repos and have published npm modules:

  • xtermjs/xterm-addon-attach
  • xtermjs/xterm-addon-fit
  • xtermjs/xterm-addon-fullscreen
  • xtermjs/xterm-addon-search
  • xtermjs/xterm-addon-terminado (discontinue in favor of community addon?)
  • xtermjs/xterm-addon-web-links
  • xtermjs/xterm-addon-winpty-compat
  • xtermjs/xterm-addon-zmodem (discontinue in favor of community addon?)

This will have many benefits like:

  • Reduce noise in core repo by consolidating issues
  • Reduce dependencies/amount of code in core repo
  • Simplify build process - the demo will still depend on some but they won't need to be built

Moving these into their own repos should also encourage us to open up the API so these core addons are no longer leveraging private API that could break. This would also be a good opportunity to add an API test suite which is a set of tests which use only the public API.

Thoughts

  • Should we use "engines": { "xterm": "..." } in package.json? We probably can't pull this out in a nice way without enforcing how things are built.
  • We could list addons published to npm by searching "xterm-addon-" and recommending that naming convention?
  • The component API being discussed in Consider adding a component API #808 is really more of a rendering API which can have more thought put into it after Buffer performance improvements #791

@Tyriar
Copy link
Member

Tyriar commented Jun 3, 2018

Had a chat with @mofux on Slack, we could do dependencies between addons by declaring the dependency names on ITerminalAddon/ITerminalAddonConstructor that would be used to block activation until deps are met. A peer dependency could be used to help ensure the dependency is installed.

@ghost
Copy link

ghost commented Jun 3, 2018

The min and/or max version checks are redundant when using the package.json peer dependency option? I'd prefer to let npm handle semver ranges i.e. 3.4.0 - 3.5.0 over adding { min: [3,4,0], max: [3,5,0] } to my add-on's class. Doing dependency mangement seems to be at odds with the unix philosophy of do one thing and do it well.

@ghost
Copy link

ghost commented Jun 3, 2018

How would you disable an add-on?

@Tyriar
Copy link
Member

Tyriar commented Jun 3, 2018

The min and/or max version checks are redundant when using the package.json peer dependency option

Further chats with @mofux came to the same conclusion 😉

How would you disable an add-on?

You can't do that now, but if we wanted to support that it would probably look something like this:

class Terminal {
  disposeAddon<T extends ITerminalAddon>(addonConstructor: ITerminalAddonConstructor<T>);
}

interface ITerminalAddon {
  dispose?(): void;
}

@ghost
Copy link

ghost commented Jun 3, 2018

If I wanted to develop an add-on with optional dependency on other add-ons, it would be nice to be able to call getAddon('someOptionalAddon') to get an add-on instance. With that in mind, I'd prefer the following for registering.

class FitAddon implements ITerminalAddon {
  name: 'fit'
  fit(): void
}
xterm.useAddon(FitAddon, function optionalCallback(fit) {
  fit.fit();
});

// And maybe allow an optional name argument
xterm.useAddon('myfit', FitAddon); // Using the name 'fit' would throw here

const fit = xterm.getAddon('myfit');

An overloaded getAddon would work just as well.

@Tyriar
Copy link
Member

Tyriar commented Jun 4, 2018

@pro-src this is how I'm imagining inter-dependencies working:

// fit.ts
import { ITerminal, ITerminalAddon } from 'xterm-base';
class FitAddon implements ITerminalAddon {
    // Still not 100% sure if we need this
    name: 'fit'
    constructor(private _terminal: ITerminal) {
    }
    fit(): void {
        // Do stuff with this._terminal
    }
}

// myFit.ts
import { ITerminal, ITerminalAddon } from 'xterm-base';
import { FitAddon } from 'xterm-addon-fit';
class MyFitAddon implements ITerminalAddon {
    name: 'myFit'
    // Will peerDependencies just work here and allow access to them?
    dependencies: [ FitAddon ]
    constructor(private _terminal: ITerminal) {
    }
    myFit(): void {
        const fit = this._terminal.getAddon(FitAddon)
    }
}

// app.ts
import { ITerminal, ITerminalAddon } from 'xterm-base';
import { FitAddon } from 'xterm-addon-fit';
import { MyFitAddon } from 'xterm-addon-myfit';
// FitAddon will initialize first, MyFitAddon will follow since it's
// dependencies are met
const term = new Terminal({}, [MyFitAddon, FitAddon]);
term.getAddon(FitAddon).fit();
term.getAddon(MyFitAddon).myFit();

@jerch
Copy link
Member

jerch commented Jun 4, 2018

LGTM 👍

Just a few questions that pop into my mind:

  • The interface looks much like composition based. Since Typescript introduced some neat OOP features, are we able to use inheritance as well? Would a class XYAddon extends ZAddon ... still register correctly? If so under which name?
  • Is an addon extensible in place, thus changing lets say fitAddon and not breaking with the dependencies of others? Not sure if we really need this, I've seen plugin systems that allow "injections" while keeping everything else intact.

@ghost
Copy link

ghost commented Jun 4, 2018

@Tyriar

this is how I'm imagining inter-dependencies working: ...

cool, that does seem to work for optional deps as well.

const deps = [FitAddon];
Try { deps.push(require('optionalDep')); } catch(e) {}

@Tyriar
Copy link
Member

Tyriar commented Jun 4, 2018

Would a class XYAddon extends ZAddon ... still register correctly? If so under which name?

Yeah I don't see why that wouldn't work, I was thinking the name property idea was needed to be used as an identifier so the name would be overridden by XYAddon.

Is an addon extensible in place, thus changing lets say fitAddon and not breaking with the dependencies of others? Not sure if we really need this, I've seen plugin systems that allow "injections" while keeping everything else intact.

Not totally clear to me what you mean here?

@Tyriar
Copy link
Member

Tyriar commented Jan 8, 2019

afterInit

Well this is essentially the constructor, the initial plan was to give an array of addons to Terminal.ctor but that seems to be overcomplicating things, especially since nothing will be rendered until open is called anyway.

Hmm - does an addon need cleanup room before the whole object tree gets disposed? dispose might be already to late...

Can you give an example?

@Tyriar
Copy link
Member

Tyriar commented Jan 8, 2019

Also I've been thinking maybe we don't even need Terminal.disposeAddon and Terminal.getAddon. If you want to manage the addon's lifecycle yourself (disabling at runtime) or need to interact with it later (eg. search), just keep a reference from loadAddon, Wrapping the dispose function on load makes disposeAddon redundant imo.

@jerch
Copy link
Member

jerch commented Jan 8, 2019

Can you give an example?

Well I have currently no idea of the dispose order, so this is quite theoretical. What if an addon relies on another component state (something internal or another addon), and has to cleanup things, but the other component is already gone? Example that comes to my mind is an addon that serializes the current terminal state to be able to resume from later on.
With a beforeDispose (could be triggered as first task of terminal.dispose) any addon can safely prepare for the upcoming dispose while the env is still intact.

@Tyriar
Copy link
Member

Tyriar commented Jan 8, 2019

Example that comes to my mind is an addon that serializes the current terminal state to be able to resume from later on.

Hmm, let's add if someone requests 🙂, that might be needed if we add addon dependencies

@mofux
Copy link
Contributor

mofux commented Jan 11, 2019

@Tyriar Sorry, I forget about the convenience of typed config options in my previous post. Couldn't we let the user instantiate the addon, and then let him pass the addon instance to term.loadAddon?

// addon definition
class MyAddon implements ITerminalAddon {
  
  // addon constructor
  constructor(config: IMyAddonConfig) {
  }

  // called when initiating the plugin
  onLoad(terminal: Terminal) {
  }

  // called when term.open() was called
  onOpen(terminal: Terminal) {
  }

  // called on term.dispose()
  dispose(terminal: Terminal) {
  }

}

// create addon instance (note: the consumer instanciates the addon, not us)
const myAddon = new MyAddon({ /*config*/ });
term.loadAddon(myAddon);

So onLoad would essentially be the hook that would handle what was previously done in the constructor. This would also have the advantage that one could create a class-less addon at runtime (not sure if it's useful though):

term.loadAddon({
  onLoad(terminal) {
  },
  dispose(terminal) {
  }
});

Update
Another advantage with this approach is that one could use the same Addon instance for multiple terminals. One example would be addons that maintain an internal cache, which could be easily reused with this approach (constructor called once, onLoad called for every instance)

What do you think?

@Tyriar
Copy link
Member

Tyriar commented Jan 11, 2019

Couldn't we let the user instantiate the addon, and then let him pass the addon instance to term.loadAddon?

I suppose we could do away with all the constructor stuff if we're only going to have a single Terminal.loadAddon API. Certainly seems simpler.

Another advantage with this approach is that one could use the same Addon instance for multiple terminals. One example would be addons that maintain an internal cache, which could be easily reused with this approach (constructor called once, onLoad called for every instance)

This is something I was trying to avoid, this would mean that there are some addons that work with multiple terminals and some that don't. It might also encourage doing this:

addonTerminal.__socket = socket;

Take the future webgl addon for example, it's all ready big enough without adding support to manage multiple terminals within the same addon, especially when it just works when there is only every a single terminal loaded.

How about we do something like this to disallow it?

loadAddon(addon: ITerminalAddon): void {
  if (addon.__isLoaded) {
    throw ...
  }
  ...
  addon.__isLoaded = true;
}

@Tyriar
Copy link
Member

Tyriar commented Jan 16, 2019

Idea for bundled addons: #1714 (comment)

@Tyriar
Copy link
Member

Tyriar commented Apr 7, 2019

New model is much simpler:

class Terminal {
    /**
     * Loads an addon into this instance of xterm.js.
     * @param addon The addon to load.
     */
    loadAddon(addon: ITerminalAddon): void;
}

export interface ITerminalAddon {
    /**
     * This is called when the addon is activated within xterm.js.
     */
    activate(terminal: Terminal): void;

    /**
     * This function includes anything that needs to happen to clean up when
     * the addon is being disposed.
     */
    dispose(): void;
}

This lets the embedder construct the addon however they wish, and we do away with the complexity of the ctor system and the additional methods. If someone wants to keep a reference around just hold on to the addon after you register:

term.loadAddon(new WebLinksAddon());
const attachAddon = new AttachAddon();
term.loadAddon(attachAddon);

// ...

attachAddon.attach(...);

I'm leaning towards not exposing a bunch of events on the addons themselves but instead allow addons to register their own events during the activate event. The addon author may need to check the state of the terminal before doing it's thing is all:

const addon = {
  activate(term: Terminal): void {
    if (term.element) {
      // it's open
    } else {
      // handle open event
      term.onOpen(() => ...);
    }
  }
}

@Tyriar
Copy link
Member

Tyriar commented Apr 14, 2019

It's worth pointing out that I wasn't planning on exporting addons on window (see #2015).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api type/enhancement Features or improvements to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants