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

Improved Layering and enabling TS strict #1507

Closed
Tracked by #1515
Tyriar opened this issue Jun 10, 2018 · 15 comments · Fixed by #2876
Closed
Tracked by #1515

Improved Layering and enabling TS strict #1507

Tyriar opened this issue Jun 10, 2018 · 15 comments · Fixed by #2876
Assignees
Labels
type/debt Technical debt that could slow us down in the long run
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2018

Background

While in Prague, @mofux and I got together to discuss a better architecture for xterm.js that could lead to things like pulling all DOM access away from the core, moving parts of xterm.js into a web worker, and swapping out the frontend completely. Notes from this discussion were captured in #1506

Proposal

I propose we split the codebase into distinct parts with very specific roles. This could be done all in the VS Code repo and still result in only a single xterm module being published, while also not obstructing a future where we may want to allow swapping parts out.

Here is the breakdown:

  • src/ - Contains all source code as today
    • base/ - Contains interfaces for core and frontend to communicate with each other, as well as code that is used between both of them
    • core/ - This runs in both node.js and browser runtimes. This means far better and easier testing since everything is just node.js without worrying about the DOM
    • ui/ - This code runs only in a browser, anything in xterm.js that interacts with the DOM today will move to here. That includes things like mouse wheel handling (and translating), composition helper, textarea, etc.
    • public/ - This provides an implementation of the current day xterm.js API.

Dependencies between this folders are strictly enforced:

|------------------|
|       base       |
|------------------|
    ↑       ↑    ↑
|------| |----|  |
| core | | ui |  |
|------| |----|  |
    ↑       ↑    |
|------------------|
|      public      |
|------------------|

Notice that ui does not depend on core, any dependencies between core and ui are pulled into interfaces and placed into base.

Example file structure

This tree will give you an idea of what lives where:

src/
  base/
    Types.ts (maybe this should be .d.ts?)
    Clone.ts
  core/
    Types.ts
    Core.ts
    components/
      Buffer.ts
      BufferSet.ts
      Parser.ts
  ui/
    Types.ts
    UserInterface.ts
    components/
      Accessibility.ts
      Composition.ts
  public/
    xterm.d.ts
    Terminal.ts

Composite projects

In TypeScript 3 we will get the ability to build parts of projects independently and depend on them. This would lead to improved build times but also we get the layering enforcement for free as the tsconfig.json for core for example will not contain the dom APIs, so any use of them will fail compilation.

Absolute imports

We may be able to use absolute imports when these changes happen, whenever I try to do it I run into issues with importing a file from the same folder as an absolute import. See #1314

Plan

This is quite a large shift from the way things are done now and therefore will take a fair bit of time to execute. Luckily it can be done in parts and many in parallel. It may be best to move to this first:

|------------------|
|       base       |
|------------------|
    ↑            ↑
|------|         |
| core |         |
|------|         |
    ↑            |
|------------------|
|      public      |
|------------------|

And slowly separate core from ui.

@Tyriar Tyriar added the type/proposal A proposal that needs some discussion before proceeding label Jun 10, 2018
@Tyriar
Copy link
Member Author

Tyriar commented Jun 10, 2018

See this PR for more specifics #1508

@jerch
Copy link
Member

jerch commented Jun 11, 2018

+1 for getting DOM related stuff strictly separated, it makes it possible to use core parts for offscreen things like an expect lib etc. which also implies that the core parts should be runnable in a pure nodejs env (without any browser engine stuff). This would also help to get it moved into a webworker later on (although this might need further glue code for interaction).

I wonder at what level of integration you intend to build the later API - will this become framework like where people can pick & alter stuff and just mount all together or will it still form the current more "monolithic" API? If we go with the framework idea and expose most of the internals we may want to deliver a default "easy to go" preconfigured setup as well (might end up delivering framework parts and a "monolithic" API, so not sure what works best).

@jerch
Copy link
Member

jerch commented Jun 11, 2018

To illustrate the framework/component idea further, take some buffering base class (no DOM) and add the following:

  1. a parser (no DOM)
  2. an escape code input handler (no DOM)
  3. optional: user input handler (keyboard + mouse, most likely DOM related)
  4. optional: an output renderer (most likely DOM related)

1 + 2 would form an offscreen terminal capable to deal with scripts/apps expecting a terminal without any interaction, with 3 added it would also be able to handle interaction. With 4 it would form a complete terminal with output representation.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 11, 2018

If we go with the framework idea and expose most of the internals we may want to deliver a default "easy to go" preconfigured setup as well (might end up delivering framework parts and a "monolithic" API, so not sure what works best).

Less API is always better as it means more flexibility for us, having said that we definitely need to add a bunch of buffer APIs as addons rely on them. We'll see what works, this model gives us the flexibility of doing so in the future, my thinking was we provide the public, and maybe @mofux maintains a monaco-xterm in the future?

@Tyriar
Copy link
Member Author

Tyriar commented Jun 11, 2018

which also implies that the core parts should be runnable in a pure nodejs env (without any browser engine stuff)

This is the major win for me, not needing to worry about DOM and being able to do end-to-end tests of the core will be very valuable.

@mofux
Copy link
Contributor

mofux commented Jun 11, 2018

Nice seeing things getting into shape, I like the proposed design!

@jerch
I'd expect that core would provide methods for dispatching keyboard and mouse events that would take our own IMouseEvent and IKeyboardEvent.

IKeyboardEvent would be a simplified version of a DOM KeyboardEvent.
IMouseEvent would be a simplified version of a DOM MouseEvent, but with the mouse coordinates already translated to a row / column system.

@Tyriar
Took me a while getting used to the folder naming, but if I got it right, the folder names could also translate to:
base --> common or shared
core --> backend
ui --> frontend

I guess it's because the words base and core are synonyms for each other in my mind 😅Maybe changing base to common or shared would make the purpose clearer?

@AndrienkoAleksandr
Copy link
Contributor

It's a really cool idea. If this will be really implemented you can easy change render to another one. Cloud 9 terminal for example it's a control sequence parser(seems old xterm) + ice editor(like render). If you really do good xterm.js splitting you can easy, for example, use Monaco editor to render terminal.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 12, 2018

I guess it's because the words base and core are synonyms for each other in my mind 😅Maybe changing base to common or shared would make the purpose clearer?

Good idea, I stayed away from frontend/backend because it's not clear what the capitalization should be. Also I like core/ui as it's very clear what that means imo. So let's go with common instead of base then? That's the name used in both VS Code and chromium for similar concepts.

@mofux
Copy link
Contributor

mofux commented Jun 12, 2018

@Tyriar Yes, renaming base to common would fully satisfy my brain 😅

@Tyriar
Copy link
Member Author

Tyriar commented Jul 3, 2018

To be clear, there's a little missing information in this diagram:

|------------------|
|   base/common    |
|------------------|
    ↑       ↑    ↑
|------| |----|  |
| core | | ui |  |
|------| |----|  |
    ↑       ↑    |
|------------------|
|      public      |
|------------------|

ui does not depend on core, but it does own an implementation of ICore which lives inside common/ and operate on it directly. This is not the case the other direction; core will fire events that are listened to by ui, core does not own a UserInterface (ie. IUserInterface lives inside ui/, not common/).

export class Core {
    public f() {
        // Fire event on Core
        this._parser.on('refresh', d => this.emit('refresh', d));
    }
}
export class UserInterface {
    public f() {
        // Core communicating with UserInterface
        this._core.on('refresh', d => renderer.refresh(d));
    }

    public x() {
        // An example of direct communication to Core inside ui/
        this._core.send(this._arrowSequences());
    }
}

Here's what I see the implementation of public/Terminal looking like:

import { ICore } from '../common/Types';
import { Core } from '../core/Core';
import { IUserInteface } from '../ui/Types';
import { UserInteface } from '../ui/UserInterface';

export class Terminal {
    private _core: ICore;
    private _userInteface: IUserInterface;

    constructor(opt) {
        this._core = new Core(opt);
    }

    public open(element: HTMLElement) {
        this._userInterface = new UserInterface(this._core, element);
    }
}

Just went through all the files and I think these are the folders we want them in:

typings/
  xterm.d.ts (maybe live in src/public eventually?)
src/
  common/
    CircularList.ts
    Clone.ts
    EscapeSequences.ts
    EventEmitter.ts
    Lifecycle.ts
    Platform.ts (previously Browser.ts, even though this touches web APIs it works across platforms)
    TestUtils.test.ts
  core/
    Buffer.ts
    BufferSet.ts
    Charsets.ts
    CharWidth.ts
    Core.ts (previously parts of src/Terminal.ts)
    EscapeSequenceParser.ts
    InputHandler.ts
    Keyboard.ts
    Mouse.ts
    TestUtils.test.ts
    Types.ts (FLAGS from renderer/Types.ts should move here)
  ui/
    renderer/ (entire folder should live here)
      CharAtlasGenerator.ts (previously shared/atlas)
    mouse/
      AltClickHandler.ts (previously handlers/AltClickHandler.ts, should eventually live in an addon)
      Linkifier.ts
      MouseHelper.ts
      MouseZoneManager.ts
      SelectionManager.ts
      SelectionModel.ts
    AccessibilityManager.ts
    CharMeasure.ts
    Clipboard.ts
    CompositionHelper.ts
    Lifecycle.ts
    RenderDebouncer.ts
    ScreenDprMonitor.ts
    Strings.ts
    TestUtils.test.ts
    Types.ts
    UserInterface.ts (previously parts of src/Terminal.ts)
    Viewport.ts
  public/
    Terminal.ts (src/Terminal.ts should eventually be moved completely into this)

Also to clarify this as I confused myself just now, you may ask why not something like this:

|------------------|
|       core       |
|------------------|
  ↑           ↑  
  |       |--------|
  |       |   ui   |
  |       |--------|
  |           |   
|------------------|
|      public      |
|------------------|

The reason is because the eventual goal is to allow core to run in a web worker. That's the sole reason this common module exists. In such a world, public/Terminal.ts will not actually be used but another implementation will be done that enables communication between core and ui. This is what that would look like:

|------------------|                          |------------------|
|   base/common    |                          |   base/common    |
|------------------|                          |------------------|
    ↑           ↑                                 ↑           ↑
|------|        |                             |------|        |
| core |        |                             |  ui  |        |
|------|        |                             |------|        |
    ↑           |                                 ↑           |
|------------------| ------postMessage------> |------------------|
|    coreworker    |                          |     uiworker     |
|------------------| <--worker.postMessage--- |------------------|

@mofux
Copy link
Contributor

mofux commented Jul 3, 2018

That's perfect, structuring it this way is definitely the way to go IMHO 👍. It also supports the ideas raised in #1515 very well.

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Sep 17, 2018
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Sep 17, 2018
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Sep 18, 2018
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 23, 2018
Belong here as it interacts with DOM

Part of xtermjs#1507
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 24, 2018
We changed plans from moving web worker compatible code to shared/
to enforcing layers using core/, common/, ui/ and public/. This
change moves files back from the shared/ dir to clean up the
codebase.

Part of xtermjs#1507
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Jun 23, 2019
This also fine tunes some of the data events to not clear selection
and scroll to the bottom of the viewport anymore.

Part of xtermjs#1507
Fixes xtermjs#2112
@Tyriar
Copy link
Member Author

Tyriar commented Jun 23, 2019

Some things to do before this is checked off:

  • Go through constructors to make sure things weren't left behind that are now in services (such as scrollToBottom, handler, etc.)
  • Verify no bad TODOs are slipping into code base

@Tyriar Tyriar added type/debt Technical debt that could slow us down in the long run and removed type/enhancement Features or improvements to existing features labels Oct 7, 2019
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 8, 2019
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Nov 15, 2019
Tyriar added a commit to Tyriar/xterm.js that referenced this issue Apr 26, 2020
@Tyriar Tyriar added this to the 4.6.0 milestone Apr 26, 2020
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.

4 participants