-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New addon API #2065
New addon API #2065
Conversation
Part of xtermjs#1128
We could release this in v3 if we don't break addons like this
Because addons build on top of the API it needs to live in public, the main reason for this is because the implementation of buffer differs on public/Terminal and src/Terminal.
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.
LGTM 👍 - just a few questions from my side.
src/ui/AddonManager.ts
Outdated
} | ||
|
||
public dispose(): void { | ||
for (let i = this._addons.length - 1; i >= 0; i--) { |
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.
Reversed disposing - should do the trick for most linear dependencies 👍
(Guess we will never see cycling deps, they are a nightmare on its own)
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.
Yeah this is something we can look into supporting as it comes up, not supported for now
src/ui/AddonManager.ts
Outdated
instance.activate(<any>terminal); | ||
} | ||
|
||
private _wrappedAddonDispose(loadedAddon: ILoadedAddon): void { |
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.
Guess Ive overlooked it - where are the addon disposals hooked into the terminal dispose chain?
Also does the API allow to do nasty things like terminal.dispose()
within addon.dispose
?
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.
Guess Ive overlooked it - where are the addon disposals hooked into the terminal dispose chain?
It just moved to dispose in ./public/Terminal
as the addon manager now lives there which is much nicer (as they build on the API, not TerminalCore
).
Also does the API allow to do nasty things like terminal.dispose() within addon.dispose?
Well you could as Terminal.dispose
is part of the API, no way around that really nor should we try imo. I see a possible infinite loop because of that, I think just moving isDisposed = true above the addon.dispose call should fix that.
The fullscreen addon will be removed when we transition over as it does so little, for fit it can be converted after the renderer API is done in #2005 |
I added back the old addons so we can ship the new API in v3, I think we should move ahead with this so that we can start converting the other addons over to the new API incrementally.
Fixes #1128
Related #2076
The API
The final API ended up being extremely simple:
In fact the only interesting part about the new addon API is how the addon's
dispose
method gets wrapped once loaded so that it can unload itself from the Terminal when called.Terminal events
The idea of exposing a more rich API with events that get fired on the addon itself came up, but I'm leaning towards having addons handle that themselves, at least for now. For example if the addon only works after open it could check
Terminal.element
:Example usage
The usage from the demo is close to this:
Notice we keep a reference to
attachAddon
as we might want to dispose it later (detach).Addon implementations