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

Fixed missing attach for parse function. #166

Merged
merged 1 commit into from
Aug 2, 2016

Conversation

alansouzati
Copy link
Contributor

Fixes #164.

It turns out that if I add attach function everything works as expected because now UI module has a parent inside the prompt function.

@dthree
Copy link
Owner

dthree commented Aug 2, 2016

Good find. 👍

@dthree dthree merged commit 2079844 into dthree:master Aug 2, 2016
@alansouzati
Copy link
Contributor Author

Cool, thanks for the fast follow up. Could please release a new version of vorpal? I'm kind of dependent on this fix to release our new CLI 💃

@dthree
Copy link
Owner

dthree commented Aug 2, 2016

Pushed to v1.11.3. 🎉

@alansouzati
Copy link
Contributor Author

🍻

@ruyadorno
Copy link

@alansouzati and @dthree just a small follow up from this PR 😊

I manage an internal cli app that used to rely on vorpal.parse mechanism to just forward commands to its vorpal actions and exit the process right away.

After this PR landed, this "command forwarding" flow is not so nice as it used to be, as the process will be kept alive by having an attached ui and users now need an extra CTRL+C to exit the app after a command ran - in the meantime I found a solution by monkey-patching vorpal.ui.attachto preserve the same functionality as before. e.g:

if (process.argv.length > 2) {
    cli.ui.attach = () => {};
    cli.parse(process.argv);
}

So my question is: Was our previous usage a non-standard and therefore non-supported way of consuming the vorpal API? Is there a better/correct syntax to implement the same functionality (non-interactive commands that just execute their logic and exit right after).

Any insights would be very much appreciated, thanks for your time!

@alansouzati
Copy link
Contributor Author

I'm facing the same issue. See this PR I sent:

#169

@dthree
Copy link
Owner

dthree commented Aug 6, 2016

Hmmm... okay. I see this. More in the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants