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

Expose typed options from OptionsService through terminal.options #3448

Merged
merged 8 commits into from
Oct 22, 2021

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Sep 1, 2021

This PR makes it possible to use typed options in the terminal addons. Much or less the example from one of the related issues.

// before
term.setOption('rendererType', 'dom');

// after
term.options.rendererType = 'dom';

As far as I am aware, this doesn't introduce breaking changes (yet).

There are some following up possibilities:

  • Align the ITerminalOptions in typings again with ITerminalOptions in code. That is, remove the undefined ? from the options, and make the constructor take a Partial<ITerminalOptions> instead of ITerminalOptions. We can assure that all options are filled in with their default values if the options are passed through to OptionsService. This would be a breaking change whatsoever, but allows to remove a lot of undefined checks.
  • Start replacing getOption and setOption in the codebase to use the getter and setters of ITerminalOptions.
  • Consider to deprecate and remove the getOption and setOption methods in favor of getters and setters.

Related: #2522, #3369

@Tyriar Tyriar added this to the 4.15.0 milestone Oct 22, 2021
@Tyriar
Copy link
Member

Tyriar commented Oct 22, 2021

Added @deprecated to old APIs, labelling with breaking change to remind me to note it in release notes (even though it's not actually breaking).

@Tyriar Tyriar added the breaking-change Breaks API and requires a major version bump label Oct 22, 2021
src/common/services/OptionsService.ts Outdated Show resolved Hide resolved
typings/xterm.d.ts Show resolved Hide resolved
Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks I've wanted this for quite a while 🎉

@Tyriar Tyriar merged commit 6978faf into xtermjs:master Oct 22, 2021
@jerch
Copy link
Member

jerch commented Nov 15, 2021

This PR creates a significant perf decrease in common modules, which have to ask option service for options (e.g. Inputhandler.print alone decreases by ~20%).
Reason behind:

get: () => {
if (!(propName in DEFAULT_OPTIONS)) {
throw new Error(`No option with key "${propName}"`);
}
return this._options[propName];
},
set: (value: any) => {
if (!(propName in DEFAULT_OPTIONS)) {
throw new Error(`No option with key "${propName}"`);
}

Fix: dont use getter/setter in fast path code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Breaks API and requires a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants