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

Change to xterm.js #1275

Closed
mofux opened this issue Dec 19, 2016 · 68 comments
Closed

Change to xterm.js #1275

mofux opened this issue Dec 19, 2016 · 68 comments
Labels
💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper

Comments

@mofux
Copy link

mofux commented Dec 19, 2016

Hi, after evaluating several JS libs for terminal emulation to improve the performance of hyper, I found that https://github.com/sourcelair/xterm.js would probably better suite this project.

  • 👍 It is actively maintained.
  • 👍 Used by Visual Studio Code and maintained by @Tyriar , who would probably welcome some combined efforts on fixing bugs and improving xterm.js
  • 👍 It doesn't require to sign an NDA to contribute
  • 👍 It doesn't isolate the terminal DOM inside an iframe, which makes it much easier to hack and style
  • 👍 It has solved some of the problems we are having with hterm currently, e.g. IME and CJK character support
  • 👎 It is currently being migrated to typescript, which might not be everyone's favourite flavour
  • 👎 It feels slightly less responsive at the moment, I think this is caused by the queue mechanism that tries to schedule big ui updates and introduces some ms lag - but it can be optimized

I did some quick tests replacing hterm with xterm.js and the performance seems level. I was also able to quick and dirty put in some of the features we get from hterm, e.g. changing the cursor color on bell.

If you like I can start working on pull request that replaces the current implementation with xterm.js, but I'd like to see some thumbs up for that first :neckbeard:

@timothyis timothyis added 💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper labels Dec 19, 2016
@timothyis timothyis changed the title [ENHANCEMENT] Change to xterm.js Change to xterm.js Dec 19, 2016
@rauchg
Copy link
Member

rauchg commented Dec 19, 2016

When I first tested it it had some serious problems in terms of support of terminal features and slower performance. Would be useful to revisit this now, though

@mofux
Copy link
Author

mofux commented Dec 19, 2016

Agree, would actually be good to have some reproducible test cases to compare both against each other. Like the vtop or emoji test for non standard-width characters (which is broken in xterm and hterm) and locale specific tests, as well as tests for unicode, performance (seq 1000000) etc... I will try to create a couple of test files and run them to compare the output.

@tbodt
Copy link
Contributor

tbodt commented Dec 20, 2016

It also may fix #270.

@tbodt
Copy link
Contributor

tbodt commented Dec 20, 2016

Actually it won't, it has no support for cursor shapes at all. It would be a lot easier to fix #270 though.

@MatthiasWinkelmann
Copy link
Contributor

@mofux I recently generated the stats at #687 (comment). It's a really somewhat terrible ruby script but I could put it in a gist if you're interested.

It's actually a bit involved to measure output lag (from the outside) because the actual commands finish long before the output arrives.

@tbodt
Copy link
Contributor

tbodt commented Dec 21, 2016

#1286 (comment)

There are a number of other styling things I'd like to fix, such as underline/i-beam cursors not blinking, but the root cause is hterm. SO CAN WE PLEASE FOR THE LOVE OF GOD USE A JAVASCRIPT TERMINAL LIBRARY THAT DOES NOT REQUIRE SIGNING AN NDA TO CONTRIBUTE TO!!!!!!11!!1 #1275

@Tyriar
Copy link
Contributor

Tyriar commented Dec 21, 2016

FYI, the main issues on xterm.js to watch regarding perf are:

It might be worth holding out on migrating until these 2 are tackled if the laggy input is too much of a regression (likely some time in January of February).

@SirTimmyTimbit
Copy link

I say we wait then. We finally have a good looking terminal on Windows, I'd be sad if it turns laggy :(

@mofux
Copy link
Author

mofux commented Dec 26, 2016

I agree. Thanks @Tyriar for giving us a rough roadmap. I'll follow the issues closely and catch up again once they have been resolved.

@dionisokan
Copy link

+1 for switching to xterm.js. As an Asian user, I would be very happy to see correctly rendered CJK characters and support for IME, as for now they mess up any other output in hyper.

@rauchg
Copy link
Member

rauchg commented Dec 29, 2016

@dionisokan can you describe the issues you have with the IME support in 1.0? cc @matheuss

@dionisokan
Copy link

@rauchg I have uploaded two GIFs (iTerm 2 & Hyper 1.0) depicting Chinese IME usage under iTerm 2 and Hyper 1.0, with the same font configuration in both terminal emulators.
I typed zhongwenceshi in order to get 中文测试. As the GIFs show, the iTerm one is normal, and what it should be, while the Hyper one is unacceptable.
Thanks in advance for looking into this problem. Also, this is likely the same issue as #149.

@rauchg
Copy link
Member

rauchg commented Dec 30, 2016

So helpful @dionisokan, thank you

@dionisokan
Copy link

I made another test. Using Chinese locales, so GNU softwares on my Mac shows in Chinese if possible. My nano is in Chinese, and here goes the result of a random nano somethingnew.text:

  • under iTerm 2, which is normal:
    image

  • under Hyper 1.0, which is abnormal:
    image

Would like to help if further testing is requested.

@Tyriar
Copy link
Contributor

Tyriar commented Dec 31, 2016

The xterm.js performance issues should be fixed in the next release, see microsoft/vscode#17875 for what's done and what's left. We're targeting Feb 6 for v2.3.0.

@avindra
Copy link

avindra commented Jan 3, 2017

An interesting benchmark would be to see how it handles yes.

Currently, hyper becomes totally unusable for 10-20 seconds after running yes

@Tyriar
Copy link
Contributor

Tyriar commented Jan 3, 2017

With the xtermjs/xterm.js#438 branch which is pending review, yes is silky smooth after the initial ~2 seconds of filling of the buffer (created xtermjs/xterm.js#444 to look into this). We don't currently sync with the backing program though which could cause issues if the app is sending too much text to process (tracked in xtermjs/xterm.js#425).

@rauchg
Copy link
Member

rauchg commented Jan 3, 2017

@avindra the question is whether that's a problem with the current RPC / redux dance, rather than hterm itself.

@rauchg
Copy link
Member

rauchg commented Jan 3, 2017

We have some improvements coming in that front regardless, which will be good for hterm, xterm, or whatever we use in the future

@Tyriar
Copy link
Contributor

Tyriar commented Jan 10, 2017

Update on xterm.js: The big performance issues have landed (excluding xtermjs/xterm.js#447) and I've started rewriting the parser to be more performant and better organized (xtermjs/xterm.js#462). After these changes xterm.js will no longer lock up when a program gives a large amount of output, and it will finish the command a lot faster (typically 300-500% faster depending on situation):

Old (within vscode) ~11s:

out

New ~2.5s:

out-2

Note that the above does not have all the fixes included and it is also being hosted within VS Code which is quite heavy when compared to Hyper. Hyper currently acts similar (but worse) than the old xterm.js (see #94).

These changes and the larger move towards TypeScript are also bringing us closer to separating rendering logic completely (necessary for #781 and microsoft/vscode#12000).

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

@Tyriar we're working on debouncing the pty calls, I don't think the choking up is hterm's fault?

@Tyriar
Copy link
Contributor

Tyriar commented Jan 10, 2017

@rauchg if by debouncing you're talking about sending XOFF and XON to the pty then fixing that would fix the lock up issue, it won't fix processing or rendering times though. The above doesn't actually include the XOFF/XON catch up support just yet (xtermjs/xterm.js#447), it's just that rendering isn't as taxing anymore.

If you did add catch up via XOFF/XON, but did not improve rendering or parsing performance the commands will be smooth but take a very long time (because you're still doing all the same stuff, it's just spaced out more). Hope that made sense, I've been digging into this stuff a lot in the last 2 weeks 😄

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

I see @Tyriar, thanks for the info re: XOFF. That said, with the spacing out, the terminal feels much better with long output streams. @hharnisc you might be interested in this ^

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

Seems like they're not implemented in hterm:

/**
 * Transmit On (XON).
 *
 * Not currently implemented.
 *
 * TODO(rginda): Implement?
 */
hterm.VT.CC1['\x11'] = hterm.VT.ignore;

/**
 * Transmit Off (XOFF).
 *
 * Not currently implemented.
 *
 * TODO(rginda): Implement?
 */
hterm.VT.CC1['\x13'] = hterm.VT.ignore;

@rauchg
Copy link
Member

rauchg commented Jan 10, 2017

@Tyriar I took another look at xterm.js the other day, but it'd be a regression for us in two fronts:

  • special characters like the ones that vtop use didn't render correctly for me (maybe you've addressed this)
  • composition / emojis dialog was not working for me.

I tried those two with master and latest stable vs code.

@ppot
Copy link
Contributor

ppot commented Feb 12, 2017

@mofux If you have a good way to implement the control flow in xterm. Go to inside #1497

@rauchg
Copy link
Member

rauchg commented Jun 11, 2017

I'm very happy with the state of xterm.js right now. Wanted to give everyone heads-up on this thread that we are currently migrating to it.

@OmgImAlexis
Copy link

@rauchg is there a current PR we can follow for the migration?

@Stanzilla
Copy link
Collaborator

@OmgImAlexis https://github.com/zeit/hyper/tree/xterm

@reflog
Copy link

reflog commented Jul 21, 2017

Looking forward to it, thank you!

@Stianhn
Copy link

Stianhn commented Nov 2, 2017

Is there a timeline for this issue?

@chabou
Copy link
Collaborator

chabou commented Nov 2, 2017

You can already use it.
Available on canary update channel.

@vallamost
Copy link
Contributor

vallamost commented Nov 6, 2017

@chabou when on the canary channel how do you flip hyper over to xterm?

@PerStirpes
Copy link
Contributor

@isseiler the canary update channel sets xterm as the default

@lijunle
Copy link

lijunle commented Nov 6, 2017

Update your .hyper.js config file:

image

@Tyriar
Copy link
Contributor

Tyriar commented Apr 18, 2018

Close me, I'm done ✨

@mofux
Copy link
Author

mofux commented Apr 18, 2018

A long journey finally comes to an end ♥️ -- but a new journey has just started! Now that we are on xterm.js it's in our hands to improve Hyper and xterm to build the most awesome terminal in the block 🎉 Big thanks to everybody involved making this happen 💪

@mofux mofux closed this as completed Apr 18, 2018
@chabou
Copy link
Collaborator

chabou commented Apr 19, 2018

@mofux Let's do this!

@DRSDavidSoft
Copy link

@mofux So Hyper v2.0 use xterm.js? Also, what did it use before xterm.js? Did Zeit develop the terminal ui before xterm.js?

@chabou
Copy link
Collaborator

chabou commented Apr 22, 2018

@DRSDavidSoft before that we used hterm (Chromium embedded terminal), in a iframe, with a lot of monkey patches. Without any power to enhance it. It was backed by DOM. More pluggable but less perfmonant/robust/healthy than xtermjs, backed by a canvas render engine and an amazing community.

@ppot
Copy link
Contributor

ppot commented Apr 22, 2018

@DRSDavidSoft Indeed the UI is developed since the start. We where using hterm but xterm is more promising and useful since we are in contact with actives maintainers and we can reduce bugs or isolate them more easily.

@cgatian
Copy link

cgatian commented May 10, 2018

@Tyriar does this mean it should be possible to use within VS Code ?

@albinekb
Copy link
Contributor

No @cgatian, we now use the same library as in vs code do render the terminal, VS Code already have a built in terminal 👍

@spyshow
Copy link

spyshow commented Jun 6, 2018

how we can integrate hype with internal vs code terminal.
as I can see in the issues it was not possible because it was not using the xterm.js but now we are on 2.0 so how can we do it?

@mofux
Copy link
Author

mofux commented Jun 6, 2018

Hyper and vscode both use xterm.js to render the terminal, but apart from that they are two completely separate applications with different architectural designs. Both cannot be integrated into each other. But the good news is: contributing enhancements and addons to xterm.js will allow both to use them! The xterm.js crew is currently working on a plugin architecture that once landed will make it much easier for vscode and hyper to use addons written for xterm.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 Feedback Wanted Issue or PR needs input from the community! Lend your thoughts ✨ 🎨 Type: Enhancement Issue or PR is an enhancement request/proposal for Hyper
Projects
None yet
Development

No branches or pull requests