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

Create a node.js target for xterm.js #2749

Closed
JavaCS3 opened this issue Mar 4, 2020 · 24 comments
Closed

Create a node.js target for xterm.js #2749

JavaCS3 opened this issue Mar 4, 2020 · 24 comments
Assignees
Labels
type/enhancement Features or improvements to existing features
Milestone

Comments

@JavaCS3
Copy link
Contributor

JavaCS3 commented Mar 4, 2020

Details

  • OS version: node
  • xterm.js version: 4.4.0
> require('xterm')
Thrown:
ReferenceError: window is not defined
    at Object.<anonymous> (/private/tmp/test/node_modules/xterm/lib/xterm.js:1:224)
    at Module._compile (internal/modules/cjs/loader.js:778:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:789:10)
    at Module.load (internal/modules/cjs/loader.js:653:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:593:12)
    at Function.Module._load (internal/modules/cjs/loader.js:585:3)
    at Module.require (internal/modules/cjs/loader.js:692:17)
    at require (internal/modules/cjs/helpers.js:25:18)

Steps to reproduce

https://runkit.com/embed/q7w97d0b8wj2

Looks like webpack4 bug,
See: https://medium.com/@JakeXiao/window-is-undefined-in-umd-library-output-for-webpack4-858af1b881df

@Tyriar
Copy link
Member

Tyriar commented Mar 4, 2020

xterm.js won't work in node because it relies on window and the DOM, it needs a browser.

@Tyriar Tyriar closed this as completed Mar 4, 2020
JavaCS3 added a commit to JavaCS3/xterm.js that referenced this issue Mar 5, 2020
JavaCS3 added a commit to JavaCS3/xterm.js that referenced this issue Mar 5, 2020
@JavaCS3
Copy link
Contributor Author

JavaCS3 commented Mar 5, 2020

@Tyriar Though xterm.js doesn't support node env currently, it should be working at importing level. The fix is quite simple, set globalObject: 'this' in webpack output will change window to this.

I wish xterm.js will support headless mode some day.

@Tyriar
Copy link
Member

Tyriar commented Mar 5, 2020

@JavaCS3 if JavaCS3@54ec873 is all that's needed to unblock you for this then we can take that, there may be other runtime problems though? I'm also not totally sure how a headless node-only version is useful. It looks like xterm-player is a browser app?

@jerch
Copy link
Member

jerch commented Mar 5, 2020

I think we should try to get the planned separation of the core and browser parts finished. This would solve those issues and also unclutter the logic by a clean separation of business logic (term emulation) and screen output. Once we got this done, we could even create a new build target for the coreparts only, to be used as a library in nodejs (xtermlib.js, whatsoever).

@JavaCS3 I currently use core parts of xterm.js in nodejs by pulling the source into a project. This way I can access internal interfaces and build them with TS as "native source". This is ofc not a viable general solution, I can only do that since I know which parts are relatively stable or changed from release to release. Its still a major step forward compared to what we had before (around v3.12/v4) with mixed browser and core stuff all over the place.

On the other hand - messing with the global object sounds like a bad idea to me. I am pretty sure that this will lead to follow up issues and should not be part of the official build. If that's a neat quickfix for your issue at hand (@JavaCS3) - maybe maintain a fork with it applied until we got the separation finished?

@JavaCS3
Copy link
Contributor Author

JavaCS3 commented Mar 6, 2020

@Tyriar It's not that urgent for JavaCS3@54ec873
I can wait until xterm.js core parts get separated or workaround in someway.

xterm-player is a browser app.

The idea of running xterm.js in headless mode is, I want to create a terminal based live sharing stuff that allow anyone to join that sharing session at anytime. Just like a video sharing, but it's a terminal based version. I want xterm.js can generate some key frames in the backend server. If anyone joins this session, the server will just send that keyframe and the following term sequence to him. That's why I want a headless mode.

@the21st
Copy link

the21st commented Mar 12, 2020

I'd also love it if xterm supported node. I originally thought node is supported, since xterm can already run in "headless" mode: #266 , without being attached to a DOM.

I tried a simple workaround in my code, just setting global.window = undefined before require('xterm') and that fixed the ReferenceError and everything is working as expected. It's just a pain to have to set that everywhere, including in tests.

Awesome work on xterm by the way 🙂

Edit: this is what got me to think Node.js should work 🙂 https://github.com/xtermjs/xterm.js/blob/master/src/Terminal.ts#L70-L71 :

// Let it work inside Node.js for automated testing purposes.
const document = (typeof window !== 'undefined') ? window.document : null;

@jerch
Copy link
Member

jerch commented Mar 12, 2020

I tried a simple workaround in my code, just setting global.window = undefined before

Thats a neat trick to get it loading. Also most of the core parts should work, thats basic I/O (write and onData), basic parser stuff and reading from the buffer. But I am not sure if all browser access code is already hidden behind .open or behind proper browser service checks, I remember still getting document errors withterm.write tests months ago w'o touching .open.

As I wrote above, the untangling is on the way, but not yet finished. If someone cares enough - the final cream topping would be a renderer abstraction for nodejs w'o any browser semantics (output to parent terminal, SDL/OpenGL).

@Tyriar
Copy link
Member

Tyriar commented Mar 12, 2020

I think we should try to get the planned separation of the core and browser parts finished.

To be clear, it was never my intention in #1507 to be able to run xterm.js within node.js. Just that we would have a clean separation in code such that we would never use the DOM within "common" (previously core) and that we could write pure node.js unit tests for core components without needing to bring in something like jsdom for them.

Once we got this done, we could even create a new build target for the coreparts only, to be used as a library in nodejs (xtermlib.js, whatsoever).

If it's easy to make an extra target (I guess with a separate .d.ts?) then we could consider it after that. However, I was also starting to rethink #1507 that maybe it should separated in a more component-based way instead of layers and then use eslint to enforce layering. This might cause issues with a node-only lib, but it would allow moving components closer together (eg. everything to do with links all in the same component as opposed to split into common and browser).

@jerch
Copy link
Member

jerch commented Mar 13, 2020

To be clear, it was never my intention in #1507 to be able to run xterm.js within node.js.

Thats clear and also would not be possible for the browser part (unless someone steps in and reimplements 50% of the codebase by other means). But wouldn't it be a shame not to make the core parts available in nodejs? By testing them in nodejs we already have unofficial node support, it just misses a neat access interface. (Edit: To emphasize this even more - strictly speaking, we kinda guarantee them to work in nodejs, but not in the browser, as we dont run the full unit tests in the browser.)

If it's easy to make an extra target (I guess with a separate .d.ts?) then we could consider it after that.

Yepp - I think all it needs, is a stripped down interface removing all browser notions. It could either be a separate build target in a subfolder with a custom package.json, or even a separate repo, that pulls from the main repo from time to time. Not sure yet which would be easier to maintain in the long run.

I was also starting to rethink #1507 that maybe it should separated in a more component-based way instead of layers and then use eslint to enforce layering. This might cause issues with a node-only lib, but it would allow moving components closer together (eg. everything to do with links all in the same component as opposed to split into common and browser).

Isnt this giving up the degree of separation we already have achieved? What real benefits do you see in such a step? To me moving parts more close together is not helpful, if the entities still show a high complexity. For a terminal emulator it seems to be logical to separate the actual emulation (business logic) from the outer world (browser), be it for testing it separately or to be able to connect it to different outer world interfaces (non browser, which is not a primary goal of term.js). Btw Bob Martin has a nice talk about this (https://www.youtube.com/watch?v=o_TH-Y78tt4).

When I reworked the mouse event handling, I modelled it to have a core and a browser part (thats the reason for creating the CoreMouseService). Imho this level of indirection has a major advantage - it separates the emulation concerns from browser related I/O. In the case of mouse events - we can test independently for the correctness of the terminal mouse protocols, and can deal with mouse events in the browser parts.

@Tyriar
Copy link
Member

Tyriar commented Apr 6, 2020

To emphasize this even more - strictly speaking, we kinda guarantee them to work in nodejs, but not in the browser, as we dont run the full unit tests in the browser.

We should probably just use a browser runner at some point, that way we can get rid of jsdom.

Isnt this giving up the degree of separation we already have achieved? What real benefits do you see in such a step? To me moving parts more close together is not helpful, if the entities still show a high complexity. For a terminal emulator it seems to be logical to separate the actual emulation (business logic) from the outer world (browser), be it for testing it separately or to be able to connect it to different outer world interfaces (non browser, which is not a primary goal of term.js). Btw Bob Martin has a nice talk about this (https://www.youtube.com/watch?v=o_TH-Y78tt4).

When I reworked the mouse event handling, I modelled it to have a core and a browser part (thats the reason for creating the CoreMouseService). Imho this level of indirection has a major advantage - it separates the emulation concerns from browser related I/O. In the case of mouse events - we can test independently for the correctness of the terminal mouse protocols, and can deal with mouse events in the browser parts.

It would be shuffling around the code base such that we still have most of the separation, just related files are more closely located. Take the mouse services as an example, here is the current layout:

browser/
  tsconfig.json
  input/
    Mouse.test.ts
    Mouse.ts
  services/
    MouseService.ts
common/
  tsconfig.json
  services/
    CoreMouseService.test.ts
    CoreMouseService.ts

This could be changed into something like this:

components/
  mouse/
    tsconfig.json
    common/
      MouseService.test.ts
      MouseService.ts
    browser/
      MouseEventUtil.test.ts
      MouseEventUtil.ts
      BrowserMouseServiceDecorator.ts

Where BrowserMouseServiceDecorator.ts augments MouseService.ts when it's attached (or some other similar design pattern, would need some thought how this works). This close proximity and better encapsulation has huge benefits in understanding the codebase, no longer are features scattered in different folders but completely contained within a single component folder. This would also allow us to do things like specify a public API in components/mouse/mouse.d.ts and only allow other modules to import from there, additionally since the component itself is a TS project, it could define it's dependencies (need to fix through files for this now: renderservice, charsizeservice, coreservice).

What we would lose here is tsconfig.json enforcing that the common part cannot import from browser, however we could enforce this using eslint (integration tests help verify too). Depending on how it's done we may lose the ability to do a node target, but it could certainly be done in a way that we could achieve that.

@Tyriar Tyriar reopened this Apr 6, 2020
@Tyriar Tyriar added type/proposal A proposal that needs some discussion before proceeding and removed as-designed labels Apr 6, 2020
@Tyriar
Copy link
Member

Tyriar commented Apr 6, 2020

Hijacking this issue so the discussion doesn't get lost 🙂

@jerch
Copy link
Member

jerch commented Apr 7, 2020

We should probably just use a browser runner at some point, that way we can get rid of jsdom.

👍

It would be shuffling around the code base such that we still have most of the separation, just related files are more closely located. ...
Depending on how it's done we may lose the ability to do a node target, but it could certainly be done in a way that we could achieve that.

Ah ok. Well, this new folder structure actually sounds good to me. About a node target - imho a clever bundler with treeshaking (like webpack) still could extract things in a certain way that excludes browser-only stuff. But this kinda needs discipline during coding to not mixup things too much. Would help here a coding convention like a file separation of browser stuff? (Thats my only "fear" by moving things closer together - ppl might end up with only one file with no browser/non-browser separation at all).

@Tyriar
Copy link
Member

Tyriar commented Apr 7, 2020

@jerch VS Code uses eslint to enforce no browser imports in the browser layer. It's doesn't work in 100% of cases though afaik. We could do multiple tsconfig projects but that's probably overcomplicating things. The eslint rule done right will probably be sufficient.

@Tyriar Tyriar changed the title ReferenceError: window is not defined when require('xterm') in node Create a node.js target for xterm.js Apr 7, 2020
@Tyriar
Copy link
Member

Tyriar commented Apr 25, 2020

I did some more thinking on this. The component model I laid out above might end up being a bad thing if the focus is to ship a node target as then we'd need to assemble many component projects and each component would have 2 or 3 tsconfigs. Here's an alternative proposal:

src/
  common/        This is the base library that both `xterm-core` and `xterm` depend on
    buffer/
    data/
    ...
    Terminal.ts  src/Terminal without the browser parts
  core/         This project generates `xterm-core`, essentially just containing its public API
    Api.ts       public/Terminal.ts without the browser parts
  web/           This is everything except for browser, it extends upon common/
    input/
    renderer/
    ...
    Terminal.ts  src/Terminal.ts, extends src/common/Terminal.ts
    Api.ts       public/Terminal.ts

One big benefits here is that things that would need to be in "common" in the old world like Links such that we can register links before attaching to the DOM, can now live safely all in browser since links should not exist in the node target. This was one of the bigger challenges to wrap my head around; how to keep code together but ensure layers are respected.

It does bring up the question again of how to separate logic for when the terminal is attached and when not. If we move everything into strict ts projects this will get handled for us as we'll be forced to do undefined checks.

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2020

InputHandler.ts is almost in common after having a go at the common/Terminal.ts idea https://github.com/Tyriar/xterm.js/tree/layers2. Also found out that IInstantiationService.createInstance apparently never worked in strict mode, we just never used it in common or browser

@Tyriar
Copy link
Member

Tyriar commented Apr 26, 2020

Good progress has been made on this. I'm not sure just yet how we'll end up publishing the module though as ./package.json in the root is xterm and points at typing/xterm.d.ts. I want to be able to publish xterm-core with a totally separate set of typings, readme and package.json while at the same time don't really want to include that in the src/ directory.

@jerch
Copy link
Member

jerch commented Apr 27, 2020

I want to be able to publish xterm-core with a totally separate set of typings, readme and package.json while at the same time don't really want to include that in the src/ directory.

I am far less into the TS compiler options than you, but wouldn't a second output folder for core stuff, that gets merged with another one holding those package declaration files, do?

Something along that:

core-package/  hold package definition files for core lib
src/
  common/
  core/
  web/
out/           everything compiled from src/
out-core/      everything compiled from src/core/

Now a pre or post bundler task could merge core-package/ on out-core/ and create a full qualified package folder, that can be tested/bundled and so on.

Other way around would be to create separate source folders with separate build targets, where one can depend on the other:

src-core/
src-web/    depends on symbols defined in src-core
out-core/
out-web/    pulls d.ts and js files from out-core

This layout would go more into the direction of two packages within one repo. Not sure about this, it might introduce issues during development like src-core always has to be built beforehand and needs proper symbol exports (prolly ending up with a public d.ts and a private version to get enough exposed for src-web).

Not sure what works best here. Can we learn from other projects here, that do internal "subpackaging" with TS?

@Tyriar
Copy link
Member

Tyriar commented Apr 27, 2020

Not sure about this, it might introduce issues during development like src-core always has to be built beforehand and needs proper symbol exports

This is how it's always been with project references, note though that web does not depend on core, but just common:

         common
         ↑    ↑
xterm-core    xterm(web)

The intent here is that core will expose it's own API just like how public/Terminal wraps browser/Terminal, hiding some internals and as a facade on top of what the actual internal API is. web then extends common/Terminal so it doesn't need to deal with the additional hop/transformations with the public core API.

I'm thinking your core-package idea might be the way to go.

@joyceerhl
Copy link
Contributor

Hi @Tyriar, I'm a dev on the Python/Jupyter VS Code extension team. We rely on the proposed vscode.window.onDidWriteTerminalData API and are hoping to reuse the xterm package for parsing the raw terminal data that comes to us from that event. (Our use case is to be able to detect when a user has run a specific command and respond to it accordingly.) Setting global.window does allow us to avoid the ReferenceErrors described in this thread but we'd much prefer not doing this in our extension code.

Is your team still planning to publish a node-compatible version of xterm.js? If so is there anything I can do to help with that process? Thank you!

@Tyriar
Copy link
Member

Tyriar commented Jan 6, 2021

Hi @joyceerhl, sounds like a valid use of the node target. This is something that our team would like to do to help out with some remote scenarios in VS Code, but I don't have a date for when this would happen currently. If you're available and willing there are definitely contributions that can be made in this area to speed things up.

The high level goal of this issue is to create a new TS project where the entry point mirrors Terminal.ts and API mirrors xterm.d.ts, but are more stripped down and don't touch the DOM or any code in src/browser.

It's a little unclear to me at the moment how much effort that will take exactly but it's waiting for someone to check it out.

@joyceerhl
Copy link
Contributor

If you're available and willing there are definitely contributions that can be made in this area to speed things up.

Happy to help! Is there a more detailed set of requirements written up somewhere that I can refer to? If not I'll give it a shot based on the high level description.

@Tyriar
Copy link
Member

Tyriar commented Jan 7, 2021

A lot of this will be exploratory, but I would start by trying to duplicate src/browser/public/Terminal.ts into a tsconfig project that does not include the dom (src/common/public/Terminal.ts is fine to get started) and strip it down so that it compiles. Once that's working we would need to create typings/xterm-core.d.ts that would be a subset of typings/xterm.d.ts, including essentially everything that isn't UI or DOM-related.

@Tyriar Tyriar added type/enhancement Features or improvements to existing features and removed type/proposal A proposal that needs some discussion before proceeding labels Aug 12, 2021
@Tyriar Tyriar self-assigned this Aug 12, 2021
@Tyriar Tyriar added this to the 4.14.0 milestone Aug 12, 2021
@Tyriar Tyriar closed this as completed Aug 12, 2021
@Tyriar
Copy link
Member

Tyriar commented Aug 12, 2021

Building upon @joyceerhl's original efforts here we have now published xterm-headless which is a headless version of xterm.js designed to run in node. Its API is a subset of the main API and based on initial explorations works well with the serialize addon (though it still needs to be packaged specially).

You can see microsoft/vscode#129207 for how it's going to be used in vscode.

@Tyriar
Copy link
Member

Tyriar commented Aug 12, 2021

@vincentwoo FYI - I think you've been after this for some time

Tyriar added a commit to Tyriar/xterm.js that referenced this issue Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Features or improvements to existing features
Projects
None yet
Development

No branches or pull requests

5 participants