Exit on parse #169

alansouzati · 2016-08-03T06:39:45Z

This PR aims to exit (detach the UI) after parser execution is complete.

dthree · 2016-08-05T06:09:16Z

Not sure this is a good move - this is not always the desired behavior. I can name a few Vorpal apps in which the app doesn't exit after parsing.

alansouzati · 2016-08-05T17:07:06Z

I see your point. See this stackoverflow that you answered:

http://stackoverflow.com/questions/33489201/is-it-possible-to-create-a-vorpal-application-which-the-commands-output-goes-di

You can have the app exit after completing the command simply by omitting vorpal.show(). Because there is no prompt, Node realizes there is nothing left to do, and the process will exit naturally.

The issue is that after my PR has been merged, there is a UI attached to the parse function. So Node will not exit naturally.

Any recommended way to fix this problem?

dthree · 2016-08-06T16:23:04Z

Okay... trying to find the best way to solve this.

I think we need to add in options to parse - something like:

vorpal.parse(process.argv, {survive: true});

With this, the default behavior will be to exit, and if a flag is passed, the exit will be omitted.

I don't really like survive though. Got a better name?

ruyadorno · 2016-08-06T16:33:41Z

👍 I like the idea of having options/config for parsing

keepAlive is certainly better than survive

dthree · 2016-08-06T16:34:23Z

Hahaha right? Just woke up, mind not functioning.

@alansouzati would you be willing to add a keepAlive flag to your PR?

alansouzati · 2016-08-06T16:57:33Z

You bet, great idea guys. Will add that tomorrow.

Alan

On Aug 6, 2016, at 9:34 AM, dc notifications@github.com wrote:

Hahaha right? Just woke up, mind not functioning.

@alansouzati would you be willing to add a keepAlive flag to your PR?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

alansouzati · 2016-08-08T18:47:22Z

Added the keepAlive, I believe we are good to go now 💃

ruyadorno · 2016-08-08T20:05:27Z

lib/vorpal.js

+          if (!options.keepAlive) {
+            // Exits the CLI context as the UI is still attached
+            this.exec('exit');
+          }


shouldn't this block be placed outside of the if (err) block?

Yeah, this should definitely run outside of the error block.

Whops. not sure what happened here. shame on me 😲

Hahaha less shame, more tests.

Fixed wrong place for check

alansouzati · 2016-08-08T20:09:57Z

fixed :)

ruyadorno · 2016-08-08T20:10:44Z

@alansouzati tried your branch with the mentioned fix from above but I still have one annoying issue, which is that the interactive shell prefix gets printed twice when I execute a command, e.g:

ad help
local@macbookpro~$

  Commands:

    help [command...]           Provides help for a given command.
    exit                        Exits application.
    ...

local@macbookpro~$

where macbookpro is my Hostname :)

ruyadorno · 2016-08-08T20:11:36Z

I think that's probably due to the fact that the ui attaches itself and then executes an exit command but I'd like your inputs on it before jumping into any conclusions...

dthree · 2016-08-08T20:11:56Z

@alansouzati if we don't resolve this shortly, I'm going to have to revert that last PR. Willing to look into additional fixes for a little though.

alansouzati · 2016-08-08T20:23:32Z

Yeah. I have the same issue.

Well, I believe printing the current delimiter is less impactful than not supporting prompt for inline commands.

See this issue for reference:

#164

alansouzati · 2016-08-08T20:24:05Z

I can try and investigate why the delimiter in being printed in this case, but I believe this is a separate issue from "exit on parse".

ruyadorno · 2016-08-08T20:27:13Z

👍 makes sense,

@dthree IMHO it's preferable to merge this the way it is instead of reverting, a fix for the extra delimiters printed can wait more

alansouzati · 2016-08-08T20:30:24Z

In the case of the delimiter, I believe if keepAlive is false we should never print it.

dthree · 2016-08-08T20:34:59Z

Okay. I would like to see if we could figure this delimiter thing out though... don't want to merge this and then have it forgotten about.

alansouzati · 2016-08-08T20:39:49Z

Do you agree with this statement: "if keepAlive is false we should never print the delimiter." ?

ruyadorno · 2016-08-08T20:41:19Z

@dthree in a second thought, maybe a revert is really a good idea in this case

seeing #172 it seems other people are suffering from the ui attach change as a breaking change

adding the keepAlive options is at best a new feature and should yield a new minor version but to be honest even with this option it still a breaking change in the API for many consuming applications out there and a new major version release would probably be better for everyone

alansouzati · 2016-08-08T20:43:42Z

agreed with @ruyadorno. maybe we need a major release as it is a breaking change.

But I believe we don't necessarily need to revert the change in Git to revert it in NPM.

alansouzati · 2016-08-08T20:46:57Z

I'm pushing the fix for the delimiter here for you guys to evaluate.

alansouzati · 2016-08-08T21:01:50Z

Ok, can you take a look now? If keepAlive is false no delimiter will be printed.

alansouzati · 2016-08-08T21:02:20Z

dist/session.js

@@ -157,7 +160,7 @@ session.delimiter = function (str) {
  if (str === undefined) {
    return this._delimiter;
  }
-  this._delimiter = String(str).trim() + ' ';


Im not sure why we are adding this empty space here.

The prompt delimiter is a very touchy subject, and if you look in the issue history, there's a lot of subtle bug fixes involved with it. Be careful before changing things, as seeming fixes can break a lot of other things in this area.

I haven't been able to break out some code to test these things as I've been swamped, so sorry for not giving too much insight. Trying to do that as soon as possible, but I think for now we should just revert instead of piling fixes on fixes which break fixes which require more fixes.

you bet. the PR is here anyways. let me know how I can help.

I did everything I could for now. I believe the ball is in your court now.

Let me know when you revert the changes so that I can re-open the issue with the ability to run prompts inside the parse context.

I understand that sometimes we need to add fixes that are not necessarily intuitive enough. I tend to add comments around it anytime I have to do things like that.

This way it helps future developers working on the library.

Thanks a lot!! 👍

dthree · 2016-08-08T21:03:55Z

But I believe we don't necessarily need to revert the change in Git to revert it in NPM.

The git repo should always reflect NPM, for users' sake.

ruyadorno · 2016-08-08T21:05:32Z

@alansouzati just updated my project with the latest changes and I can confirm that this fixes the extra delimiters printed issue 👍

thank you so much for all the work on this 😊

alansouzati · 2016-08-08T21:12:40Z

thanks for trying it out @ruyadorno.

It seems that we will need to wait until @dthree figures out the side effects of this change.

ruyadorno · 2016-08-09T17:07:25Z

@alansouzati I have yet another very tricky situation, I actually have a mixed of interactive and non-interactive (keepAlive:false) commands in my cli app - @dthree is that supposed to be a supported use case?

in this situation it would be nice to have a way to define the keepAlive option from within the command definition instead...

alansouzati · 2016-08-10T03:41:58Z

hey @dthree any updates on this?

I have few folks complaining about the grommet new sample-app hanging the UI. I really want to avoid a three step process like

grommet
new sample-app
exit

alansouzati · 2016-08-10T03:55:24Z

Actually no rush anymore. I added my fork branch in my package.json for now.

https://github.com/grommet/grommet-cli/blob/master/package.json#L27

Exit on parse

e296576

alansouzati mentioned this pull request Aug 5, 2016

Fixed missing attach for parse function. #166

Merged

ruyadorno reviewed Aug 8, 2016
View reviewed changes

Added keepAlive option.

4359f43

Fixed wrong place for check

alansouzati force-pushed the EXIT_ON_PARSE branch from 1ef3f9a to 4359f43 Compare August 8, 2016 20:09

dthree mentioned this pull request Aug 8, 2016

vorpal.parse() method has breaking changes to behaviour in 1.11.4 #172

Closed

Skip delimiter for keepAlive false

d32502d

alansouzati reviewed Aug 8, 2016
View reviewed changes

alansouzati mentioned this pull request Aug 10, 2016

Hang on grommet new sample-app grommet/grommet-cli#1

Closed

Added fix for undefined

6ff0bc5

LongLiveCHIEF mentioned this pull request Dec 3, 2016

Do not attach ui after parse #200

Merged

alansouzati added 2 commits January 5, 2017 15:03

Removed imprint support

b100c5e

removed imprint from dist folder

365d1fb

alansouzati mentioned this pull request Jun 6, 2017

package.json depends on package outside of npm grommet/grommet-cli#36

Closed

Exit on parse #169

Are you sure you want to change the base?

Exit on parse #169

Conversation

alansouzati commented Aug 3, 2016

dthree commented Aug 5, 2016

alansouzati commented Aug 5, 2016

dthree commented Aug 6, 2016

ruyadorno commented Aug 6, 2016

dthree commented Aug 6, 2016

alansouzati commented Aug 6, 2016

alansouzati commented Aug 8, 2016

ruyadorno Aug 8, 2016

Choose a reason for hiding this comment

dthree Aug 8, 2016

Choose a reason for hiding this comment

alansouzati Aug 8, 2016

Choose a reason for hiding this comment

dthree Aug 8, 2016

Choose a reason for hiding this comment

alansouzati commented Aug 8, 2016

ruyadorno commented Aug 8, 2016

ruyadorno commented Aug 8, 2016

dthree commented Aug 8, 2016

alansouzati commented Aug 8, 2016

alansouzati commented Aug 8, 2016

ruyadorno commented Aug 8, 2016

alansouzati commented Aug 8, 2016

dthree commented Aug 8, 2016

alansouzati commented Aug 8, 2016 • edited

ruyadorno commented Aug 8, 2016

alansouzati commented Aug 8, 2016

alansouzati commented Aug 8, 2016

alansouzati commented Aug 8, 2016 • edited

alansouzati Aug 8, 2016

Choose a reason for hiding this comment

dthree Aug 8, 2016 • edited

Choose a reason for hiding this comment

alansouzati Aug 8, 2016

Choose a reason for hiding this comment

alansouzati Aug 8, 2016 • edited

Choose a reason for hiding this comment

dthree Aug 8, 2016

Choose a reason for hiding this comment

dthree commented Aug 8, 2016

ruyadorno commented Aug 8, 2016

alansouzati commented Aug 8, 2016

ruyadorno commented Aug 9, 2016

alansouzati commented Aug 10, 2016

alansouzati commented Aug 10, 2016

alansouzati commented Aug 8, 2016 •

edited

alansouzati commented Aug 8, 2016 •

edited

dthree Aug 8, 2016 •

edited

alansouzati Aug 8, 2016 •

edited