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

Define a strict public API #277

Closed
Tyriar opened this issue Sep 17, 2016 · 18 comments
Closed

Define a strict public API #277

Tyriar opened this issue Sep 17, 2016 · 18 comments
Labels
type/proposal A proposal that needs some discussion before proceeding
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Sep 17, 2016

Before going v2.0.0 we should lock down the API and explicitly state what is public and what is private. Right now many of the functions are included in the API doc site but are actually private, some examples: blankLine, bind*, cursor*, ...

Not doing so makes it harder to know what you're meant to call in to as an embedder, harder to use, more likely to use private APIs what break across versions and it could slow the speed at which we develop as a result.

@Tyriar Tyriar added the type/proposal A proposal that needs some discussion before proceeding label Sep 17, 2016
@Tyriar Tyriar added this to the 2.0.0 milestone Sep 17, 2016
@parisk
Copy link
Contributor

parisk commented Sep 18, 2016

I am 👍 with this.

IMO we should start with a strict public API as small as possible. This will let us maintain backwards compatibility of it, without losing flexibility when messing with the rest of the methods.

We can then introduce new public methods on an as-needed basis afterwards.

So my initial proposal for the public API are the following methods:

Terminal.prototype

  • blur
  • clear
  • destroy
  • blur
  • focus
  • open
  • refresh
  • reset
  • resize
  • setOption
  • write
  • writeln

Maybe we should add a Terminal.prototype.getOption method also.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2016

One thing that's always bugged me with refresh is if no args are specified it should refresh the whole viewport.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2016

Here are all the calls vscode uses currently:

constructor()
open(HTMLelement)
write(string)
on('data', string => boolean)
attachCustomKeydownHandler(KeyboardEvent => boolean)
destroy()
focus()
scrollDisp(number)
clear()
refresh(number, number)
setOption(string, object)
resize(number, number)

// to check if cursorBlink is enabled, could be replaced with a getOption function
cursorBlink

// to attach focus/blur events to save state when the terminal in focused
// to explicitly set the width of the element
element

// to attach focus/blur events to save state when the terminal in focused
textarea

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2016

Adopting an _ prefix for private members would make this very clear in the code what is private and what is public.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 18, 2016

Something else that should be considered is #266. open(HTMLElement) could become open(HTMLElement?) and then have an additional attachToDom(HTMLElement) method.

It might be worth rethinking and simplifying initialization as part of 2.0.0, Is there any reason for the open not being rolled into the constructor for example?

@parisk
Copy link
Contributor

parisk commented Sep 19, 2016

I completely forgot the on/off event "handlers" and the attachCustomKeydownHandler.

scrollDisp could make it for a public API, but IMO we should change it's name to something more intuitive.

Adopting an _ prefix for private members would make this very clear in the code what is private and what is public.

I think that the best thing for now would be to just document the Public API clearly into a fresh docs.xtermjs.org. Renaming all functions at the moment is really error prone. We can do that incrementally as these functions will be "private", thus we can change them at any point 😈 .

Something else that should be considered is #266. open(HTMLElement) could become open(HTMLElement?) and then have an additional attachToDom(HTMLElement) method.

Not sure what you are talking about here. What is the open supposed to do if no HTMLElement is passed as argument.

Also would it make sense to merge open into the constructor?

@parisk
Copy link
Contributor

parisk commented Sep 19, 2016

Also we can JSDoc all private methods as @private, which is totally safe.

Last, I propose replacing the following:

  • cursorBlink
  • element
  • textarea

with:

  • getOption('cursorBlink')
  • getElement()
  • getTextarea()

which are more verbose, but at the same time more explicit and not editable (so consumers are less likely to mess things up)

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

@parisk more intuitive init logic could be:

constructor(HTMLElement?);
attachToElement(HTMLElement);

or

constructor();
attachToElement(HTMLElement);

@ebertmi
Copy link

ebertmi commented Sep 19, 2016

What should happen when attachToElement(HTMLElement); gets called more than once and maybe on different elements?

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

@waywaaard probably throw and exception

@ebertmi
Copy link

ebertmi commented Sep 19, 2016

Maybe, a detachFromElement would be good.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 19, 2016

For completeness maybe, can't think of a use case for it though.

@parisk
Copy link
Contributor

parisk commented Sep 19, 2016

What about making the attaching part of the construction (just like how CodeMirror handles this: http://codemirror.net/doc/manual.html#api_constructor)?

@parisk
Copy link
Contributor

parisk commented Sep 21, 2016

🆗 I will leave the constructor as-is until we decide on something. I propose merging .open into the constructor, unless we have a use case for not doing so.

I will start documenting at docs.xtermjs.org the rest of the methods mentioned here as the Public API for v2.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 25, 2016

@parisk on scroll, scrolling pages and the entire buffer would also be good to expose, this would be needed to allow consumers to customize keybindings, so something like:

xterm.scroll(value[, type]);
xterm.SCROLL_TYPE = LINE | PAGE | BUFFER

or

xterm.scrollLines(value);
xterm.scrollPages(value);
xterm.scrollToTop();
xterm.scrollToBottom();

@parisk
Copy link
Contributor

parisk commented Sep 29, 2016

Let's leave additional functionality outside for now. The smallest the API is the easier is to maintain. Let's support more methods on an as-needed basis.

The first version of the public API is documented at xtermjs/xtermjs.org#1 and exposes only a few methods of the Terminal constructor.

It can be previewed at https://d8q3uus5.apps.lair.io/docs/api/Terminal/.

@Tyriar
Copy link
Member Author

Tyriar commented Sep 29, 2016

@parisk I'm currently relying on scrollDisp and rows to implement scroll page, scroll to top/bottom is a little too complex for me to do without tests. If we're changing scrollDisp -> scroll we should make sure we don't want to introduce a breaking change when adding these.

@parisk
Copy link
Contributor

parisk commented Oct 3, 2016

Closing this as it got defined in xtermjs/xtermjs.org#1.

@parisk parisk closed this as completed Oct 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/proposal A proposal that needs some discussion before proceeding
Projects
None yet
Development

No branches or pull requests

3 participants