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

support nodejs REPL console inside neo-blessed #768

Closed
iurimatias opened this issue Aug 31, 2018 · 40 comments
Closed

support nodejs REPL console inside neo-blessed #768

iurimatias opened this issue Aug 31, 2018 · 40 comments

Comments

@iurimatias
Copy link
Collaborator

iurimatias commented Aug 31, 2018

Outline

Currently the embark dashboard (shown in embark run) has a log section and a input field to type commands. This input field is not very good as it doesn't support going back with a cursor to correct something, history is limited, there is no autocomplete, etc.. On the other hand embark console (available develop branch) is a full featured REPL.

The goal of this task is to support the same REPL console used in embark console in the embark dashboard. This likely means some changes will be needed in neo-blessed

Acceptance Criteria

  • The embark dashboard in embark run, the input field should disappear, and the log section should have a REPL with the same features as the one embark console
@iurimatias iurimatias added this to bounty-awaiting-approval in Bounties Aug 31, 2018
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 800.0 DAI (800.0 USD @ $1.0/DAI) attached to it.

@andytudhope andytudhope moved this from bounty-awaiting-approval to bounty (open) in Bounties Sep 4, 2018
@StatusSceptre
Copy link

Hey @pinkiebell - @lastmjs applied first here, but if you guys want to work together, we'd be happy to split the bounty for this. Lmk what you would like to do and what will work best, we're still experimenting with the Gitcoin stuff...

@andytudhope andytudhope moved this from bounty (open) to bounty (in progress) in Bounties Sep 4, 2018
@pinkiebell
Copy link

@StatusSceptre
No problem 👍
@lastmjs has the favour, let me know if you need help 😃

@vs77bb
Copy link

vs77bb commented Sep 7, 2018

Both great candidates! @lastmjs glad to see you here - don't hesitate to reach out with questions as you progress 🙂

@lastmjs
Copy link
Contributor

lastmjs commented Sep 7, 2018

Sounds good, I just started familiarizing myself with the codebase, I should make significant progress tomorrow, and don't worry I'll be getting on Slack soon

@lastmjs
Copy link
Contributor

lastmjs commented Sep 7, 2018

Update: I've gotten rid of the console input, and I have the node REPL outputting to the Log area, though jankily.

@lastmjs
Copy link
Contributor

lastmjs commented Sep 8, 2018

I want to get some thoughts on what's going on. I don't know if it's the best idea to get rid of the console section and consolidate it into the log section. The reason is that logs can come from various places in the application at various times. If the repl is inside of the log container, then what you're currently typing could be written over at any time by a random log. If we separate the log and console sections, but have the console output go into the log section like the original setup, then we get around that problem. Any objections to this?

@lastmjs
Copy link
Contributor

lastmjs commented Sep 8, 2018

To get this to work, I think I'm going to need to use a blessed.terminal, and to get that to work I need the dependency pty.js. Unfortunately, pty.js has some V8 compatibility issues (it doesn't work with my version of Node.js 10). I installed the latest Node.js 8 and the V8 compatability issues are gone.

Track this issue: chjj/pty.js#195

@pinkiebell
Copy link

@lastmjs
If you pass a handler: function(){} to blessed.Terminal, then you don't need pty.js 👍 .

@iurimatias
Copy link
Collaborator Author

@lastmjs I need to consult the team about this

@iurimatias
Copy link
Collaborator Author

@lastmjs alright it makes sense.

@lastmjs
Copy link
Contributor

lastmjs commented Sep 10, 2018

Alright, things are working pretty well. Looks like I just need to clean up and figure out styling, I'll get a PR up today to start looking at

@lastmjs
Copy link
Contributor

lastmjs commented Sep 10, 2018

I'm trying to style the terminal, but it's having some major issues. If I try to put a border directly on the terminal, the following errors get thrown:

pid 2783 listening on /home/lastmjs/development/embark_demo/.embark/embark.ipc

<--- Last few GCs --->

[2783:0x29e7c70]     5329 ms: Mark-sweep 1398.2 (1425.0) -> 1397.8 (1425.5) MB, 187.6 / 0.0 ms  (average mu = 0.118, current mu = 0.025) allocation failure scavenge might not succeed
[2783:0x29e7c70]     5524 ms: Mark-sweep 1398.5 (1425.5) -> 1398.1 (1426.0) MB, 194.0 / 0.0 ms  (average mu = 0.062, current mu = 0.002) allocation failure scavenge might not succeed


<--- JS stacktrace --->

==== JS stack trace =========================================

    0: ExitFrame [pc: 0x2af2979041bd]
    1: StubFrame [pc: 0x2af297915610]
Security context: 0x33927891e6c9 <JSObject>
    2: new Terminal [0x3c8358cb2f19] [/home/lastmjs/development/embark/node_modules/term.js/src/term.js:~182] [pc=0x2af298033a3a](this=0x3c8358cb2d21 <Terminal map = 0x3c21858fa129>,options=0x3c8358cb2f99 <Object map = 0x3c21858ffb39>)
    3: ConstructFrame [pc: 0x2af29790f147]
    4: StubFrame [pc: 0x2af29800d2c0]
    5...

FATAL ERROR: Ineffective mark-compacts near heap limit Allocation failed - JavaScript heap out of memory
 1: 0x8b5f80 node::Abort() [node]
 2: 0x8b5fcc  [node]
 3: 0xab730e v8::Utils::ReportOOMFailure(v8::internal::Isolate*, char const*, bool) [node]
 4: 0xab7528 v8::internal::V8::FatalProcessOutOfMemory(v8::internal::Isolate*, char const*, bool) [node]
 5: 0xea5152  [node]
 6: 0xeb10aa v8::internal::Heap::PerformGarbageCollection(v8::internal::GarbageCollector, v8::GCCallbackFlags) [node]
 7: 0xeb1a14 v8::internal::Heap::CollectGarbage(v8::internal::AllocationSpace, v8::internal::GarbageCollectionReason, v8::GCCallbackFlags) [node]
 8: 0xeb4345 v8::internal::Heap::AllocateRawWithRetry(int, v8::internal::AllocationSpace, v8::internal::AllocationAlignment) [node]
 9: 0xe7c824 v8::internal::Factory::NewFillerObject(int, bool, v8::internal::AllocationSpace) [node]
10: 0x111e1de v8::internal::Runtime_AllocateInNewSpace(int, v8::internal::Object**, v8::internal::Isolate*) [node]
11: 0x2af2979041bd 
Aborted (core dumped)

If I try to make the terminal the child of a box, the screen goes blank and I can't get out of it without quitting the terminal. Anyone know how to get around these issues?

@lastmjs lastmjs mentioned this issue Sep 10, 2018
@lastmjs
Copy link
Contributor

lastmjs commented Sep 11, 2018

I tracked the issue I was having above down to the height property of the terminal...seems like a height of 4% is causing the error. 5% or 10% is fine, but 4% causes the error

@lastmjs
Copy link
Contributor

lastmjs commented Sep 11, 2018

How necessary is it that the output from the repl go into the log section? The problem is differentiating, inside of the repl's writable stream, between input from the user that should be written to the one line terminal, or the actual output of the repl that should go into the log section. It gets complicated because of chunking and things like autocomplete that is an exception to the repl output (it doesn't run through the same output function that allows me to capture the output). Also, styling the terminal to be one line and never have any ill-effects from different terminal sizes and such will probably be difficult.

So, if there is any way that we could have a log section and a terminal section, with separate inputs and outputs, that would be ideal on the development side. Thoughts?

@iurimatias
Copy link
Collaborator Author

Hi @lastmjs

We discussed & analyzed this and we feel that separating the logs is not the way to go as it would cause other problems. However we experimented with this and it seems that when the input is at the right height, all the issues reported pretty much disappear. for e.g setting it to 5% for a good height pretty much fixes all issues including the overflow, except the autocomplete (if there are multiple options, they wouldn't show), however that's OK.
The ideal % seems to vary depending on the height of the terminal, so a possible solution is to detect window resizes, check the height, and then automatically set a new height for the input based on the based (can be checked with something like window-size. Let us know what you think

@iurimatias
Copy link
Collaborator Author

Another solution involves changing/improving the "css" layout of the dashboard so the input size is always correct.

@andremedeiros
Copy link
Collaborator

Hey @lastmjs, I have a minimal proof of concept that'll show you something that could make your life a lot easier. Effectively we want to keep the size of the REPL box constant. This snippet got me there (contracts is fixed, logs is flexible, REPL is fixed.) Try running this and resizing your terminal to see what I mean.

const blessed = require('neo-blessed');

const screen = blessed.screen({smartCSR: true, dockBorders: true});
screen.title = 'Embark UI';

const contractsBox = blessed.box({
  position: {
    left: 0,
    top: 0
  },
  width: '100%',
  height: 15,
  title: 'OMG',
  content: 'contracts',
  border: {
    type: 'line'
  },
  style: {
    border: {
      fg: 'green'
    }
  }
});

const logsBox = blessed.box({
  position: {
    left: 0,
    top: 16,
  },
  content: 'logs',
  width: '100%',
  height: '100%-18',
  border: {
    type: 'line'
  },
  style: {
    border: {
      fg: 'green'
    }
  }
});

const replBox = blessed.box({
  position: {
    left: 0,
    top: '100%-3'
  },
  content: 'repl',
  height: 3,
  width: '100%',
  border: {
    type: 'line'
  },
  style: {
    border: {
      fg: 'green'
    }
  }
});

screen.append(contractsBox);
screen.append(logsBox);
screen.append(replBox);

screen.render();

@iurimatias iurimatias moved this from bounty (in progress) to bounty (PR open/submitted) in Bounties Sep 14, 2018
@lastmjs
Copy link
Contributor

lastmjs commented Sep 16, 2018 via email

@gitcoinbot
Copy link

@lastmjs Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@lastmjs
Copy link
Contributor

lastmjs commented Sep 20, 2018 via email

@andremedeiros
Copy link
Collaborator

@lastmjs looking forward to it!

@lastmjs
Copy link
Contributor

lastmjs commented Sep 21, 2018

@andremedeiros Great, I see what your demo is trying to do. Though it seems easy to lose the log area entirely like that. Is that okay? Would it be better to make everything else flexible, and just keep the repl box fixed in height?

@gitcoinbot
Copy link

@lastmjs Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@lastmjs
Copy link
Contributor

lastmjs commented Sep 29, 2018

Looks like we're getting close, just a few small issues to resolve

@gitcoinbot
Copy link

@lastmjs Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@lastmjs
Copy link
Contributor

lastmjs commented Oct 5, 2018

Looking into finishing up today

@iurimatias iurimatias moved this from bounty (WIP or final PR open/submitted) to bounty (changes requested) in Bounties Oct 5, 2018
@andremedeiros
Copy link
Collaborator

Hey @lastmjs,

I gave the branch a run, and the command line feels sooooo crisp! Definitely a step in the right direction.

I am, however, having some issues with the UI re-orienting itself on my terminal, specifically on 186 cols and 97 lines, but can replicate it in other setups.

I think it could be helpful to jump into a call and maybe show you what I mean.

Damn, this feels so close I can almost taste it!

@lastmjs
Copy link
Contributor

lastmjs commented Oct 9, 2018

What time works for you? I'm at SF Bockchain Week most days this week and wasn't planning on bringing my laptop, but perhaps we can get on a call at night Pacific Time or early morning Pacific Time?

@andremedeiros
Copy link
Collaborator

I'm on EST. Tomorrow I have the whole day, from 10am EST to around 4pm EST if that helps.

If you'd prefer skipping the laptop, I'd effectively try your code with non standard sized terminals. Portrait instead of landscape, resize the window on both height and width to see some weird artifacts going on, maybe even reduce font size. I've tried all of these, and the call would be to demonstrate this.

@lastmjs
Copy link
Contributor

lastmjs commented Oct 13, 2018 via email

@lastmjs
Copy link
Contributor

lastmjs commented Oct 15, 2018

I think I could use some clarity. Do we have a design for portrait versus landscape, or a responsive design that we will be following? For example, do we want the Environment, Status, and Available Services boxes to stack vertically under the Contracts box when the screen gets too narrow? Or do we want those boxes to be scrollable when the screen is too narrow?

@lastmjs
Copy link
Contributor

lastmjs commented Oct 24, 2018

Any clarity here?

@StatusSceptre
Copy link

@andremedeiros or @iurimatias any update on this one for @lastmjs?

@iurimatias
Copy link
Collaborator Author

@lastmjs can we chat on gitter?

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 800.0 DAI (800.0 USD @ $1.0/DAI) attached to this issue has been cancelled by the bounty submitter

@StatusSceptre StatusSceptre moved this from bounty (changes requested) to bounty (completed) in Bounties Dec 11, 2018
@iurimatias iurimatias removed this from bounty (completed) in Bounties Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants