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

ITerminalOptions: Make fields required #3948

Closed
silamon opened this issue Jul 27, 2022 · 2 comments · Fixed by #4004
Closed

ITerminalOptions: Make fields required #3948

silamon opened this issue Jul 27, 2022 · 2 comments · Fixed by #4004
Assignees
Labels
breaking-change Breaks API and requires a major version bump help wanted type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@silamon
Copy link
Contributor

silamon commented Jul 27, 2022

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>.

@Tyriar
Copy link
Member

Tyriar commented Jul 28, 2022

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 Tyriar added help wanted type/debt Technical debt that could slow us down in the long run breaking-change Breaks API and requires a major version bump labels Jul 28, 2022
@Tyriar
Copy link
Member

Tyriar commented Jul 28, 2022

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

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 help wanted type/debt Technical debt that could slow us down in the long run
Projects
None yet
2 participants