xterm.js icon indicating copy to clipboard operation
xterm.js copied to clipboard

ITerminalOptions: Make fields required

Open silamon opened this issue 3 years ago • 2 comments

In #3667, all getOptions were replaced by their new equivalent: this.dimensions.scaledCharLeft = Math.floor(this._terminal.getOption('letterSpacing') / 2); became this.dimensions.scaledCharLeft = Math.floor(this._terminal.options.letterSpacing! / 2);

Mind the "!" there, which is a shortcut to let typescript know that the variable is for sure defined. In fact, that can be safely assumed since the default parameters are filled in. If the class ITerminalOptions is changed to have required fields which are not optional, it would be possible to have this.dimensions.scaledCharLeft = Math.floor(this._terminal.options.letterSpacing / 2);

It was kind of explored in #3448 that options: ITerminalOptions; can be changed to get options(): ITerminalOptions; set options(options: Partial<ITerminalOptions>): void; which would be an equivalent of the current code. Just ITerminalOptions is changed to have required fields.

There are some settings though which can be "undefined":

  • fastScrollModifier: Perhaps we can introduce "none" here
  • overviewRulerWidth: Use 0 to have this disabled?

This would also make the code more aligned, since the typings have ITerminalOptions with all fields optional and the internal ITerminalOptions have required fields.

The breaking change part here lies in the fact that addons can use ITerminalOptions as type. If this is passed as a parameter, they would need to change the parameter to Partial<ITerminalOptions>.

silamon avatar Jul 27 '22 17:07 silamon

I like it. Also yes we should get rid of undefined in fastScrollModifier and overviewRulerWidth as I'm not even sure it's possible to unset them in the current state, I like your proposals.

Let's do this as part of v5 so we can make the (minor) breaking change

Tyriar avatar Jul 28 '22 17:07 Tyriar

Let me know if you don't have time in the next week and I can pick this up

Tyriar avatar Jul 28 '22 17:07 Tyriar