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

Consider referencing xterm.d.ts from internal types to reduce doc repetition #4663

Closed
Tyriar opened this issue Aug 11, 2023 · 3 comments · Fixed by #4679
Closed

Consider referencing xterm.d.ts from internal types to reduce doc repetition #4663

Tyriar opened this issue Aug 11, 2023 · 3 comments · Fixed by #4679
Assignees
Labels
type/debt Technical debt that could slow us down in the long run

Comments

@Tyriar
Copy link
Member

Tyriar commented Aug 11, 2023

From @PerBothner's suggestion in #4519 (comment)

Right now we often either don't have docs linked to internal types (making development harder, especially for new contributors) or duplicate the docs which of course can easily suffer bitrot.

One solution is to reference xterm and then extend Pick<Terminal, 'apis'> like this:

image

If we do this we would want some rules around the usage, here are some ideas using Terminal.selectAll()/ISelectionService.selectAll() as an example.

1: Pull in any types referenced to a special types file

Example:

ApiTypes.ts

import { Terminal } from 'xterm';

// Each function used internally would have it's own type
export type IApiSelectAll = Pick<Terminal, 'selectAll'>;

Services.ts

export interface ISelectionService extends IApiSelectAll {
}

2: Pull in directly from the API

Services.ts

import { Terminal as IApiTerminal } from 'xterm';

export interface ISelectionService extends Pick<IApiTerminal, 'selectAll'> {
}

I think 1 is probably the nicest unless there are other ideas because:

  • It's obvious across the codebase what IApi* types are
  • It hides the type complexity inside the ApiTypes file
  • Extending multiple is cleaner as a result
  • No need to rename the Terminal as IApiTerminal import manually when a file hasn't referenced it yet
export interface ISelectionService extends IApiSelectAll,
                                           IApiSelectLines {
}

vs

export interface ISelectionService
  extends Pick<IApiTerminal, 'selectAll' | 'selectLines'> {
}
@Tyriar Tyriar added the type/debt Technical debt that could slow us down in the long run label Aug 11, 2023
@Tyriar Tyriar self-assigned this Aug 11, 2023
@PerBothner
Copy link
Contributor

One problem I have dealing with the code is that there are so many classes and interfaces that are variations of each other, aliases of each other, or implement/extend each other. There is a lot of abstraction - which has its benefits, but it makes the code harder to understand.

For example there are five declarations of class Terminal. In addition, there is Coreterminal, ITerminalApi, and ITerminal.

Without discussing proposal 1 on its merits (which I don't think I'm ready to do), it has the disadvantage that it increases the abstraction penalty and adds yet more type names. So that is definitely a disadvantage.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 11, 2023

I'm not sure if we document it properly but the abstraction makes a lot more sense when you look at it in terms of the layer sub-projects (which enables xterm-headless). The Terminal case is the most abstracted as it's the primary object but the rules are:

  • common/ can run in a browser or node.js
  • browser/ can only run in a browser and can import from common/
  • Anything in public/ defines API, it cannot reference anything outside of public

There's a diagram around here somewhere that shows this as a diagram that we should stick in the wiki. This pattern (and others) are also very similar to how it works in VS Code's codebase so it's pretty natural when you're coming from there which I imagine a decent chunk of contributors do.

It could always be improved and there are some efforts that are half completed and a bit messy (like the services/ folder which I've kind of changed my mind about). It's nicer when you consider it used to be a single 6000 line file without types that was a real struggle to maintain and improve.

@Tyriar
Copy link
Member Author

Tyriar commented Aug 14, 2023

At least for the Terminal objects we could pull in the whole API and implement them to get docs working:

image

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Aug 15, 2023
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/debt Technical debt that could slow us down in the long run
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants