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

Add --shell-auto-fallback #7

Merged
merged 5 commits into from May 31, 2017

Conversation

Projects
None yet
2 participants
@passcod
Copy link
Contributor

passcod commented May 30, 2017

Generates shell code that hooks into the "command not found" mechanism
and attempts to run the command with npx instead of failing. The option
has an optional argument that forces generation of a particular variant,
otherwise it attempts to autodetect the shell.

As Seen On Twitter

feat(opts): add --shell-auto-fallback
Generates shell code that hooks into the "command not found" mechanism
and attempts to run the command with npx instead of failing. The option
has an optional argument that forces generation of a particular variant,
otherwise it attempts to autodetect the shell. However, autodetecting is
actually very flaky, so it's better to always specify the argument.

To use, place this in relevant shell config file:

For Bash:

    source <(npx --shell-auto-fallback bash)

For Zsh:

    source <(npx --shell-auto-fallback zsh)

For Fish:

    source (npx --shell-auto-fallback fish | psub)

As Seen On Twitter: https://twitter.com/passcod/status/869469928474107906

@passcod passcod changed the title feat(opts): add --shell-auto-fallback Add --shell-auto-fallback May 30, 2017

@zkat

This comment has been minimized.

Copy link
Owner

zkat commented May 30, 2017

This is lovely. Do you mind adding docs about this to README.md?

@zkat zkat self-requested a review May 30, 2017

@zkat
Copy link
Owner

zkat left a comment

Docs, please!

@zkat
Copy link
Owner

zkat left a comment

Ok! I actually went and reviewed the code, which I should've done in the first place. Looks good so far, with a few changes requested.

This still needs to get documented (be sure to add it to usage in both the README and in parse-args!)

end`

module.exports = function autoFallback (shell) {
const { BASH_VERSION, SHELL, ZSH_VERSION } = process.env

This comment has been minimized.

@zkat

zkat May 31, 2017

Owner

We're restricted to node@4 or latest, so was can't do destructuring assignment 😭

@@ -18,6 +19,12 @@ updateNotifier({pkg}).notify()
main(parseArgs())

function main (argv) {
const shell = argv['shell-auto-fallback']
if (shell || shell === '') {

This comment has been minimized.

@zkat

zkat May 31, 2017

Owner

won't this always be true? parseArgs() defaults to ''

This comment has been minimized.

@passcod

passcod May 31, 2017

Contributor

requireArg is false! So sometimes the option will return null/undefined/false/whatever yargs returns for a non-provided option. But '' is falsy, so I do need to check.

This comment has been minimized.

@zkat

zkat May 31, 2017

Owner

doesn't having a default mean that you always get '' if you don't provide the option, though? I'm so confused.

@@ -62,6 +62,14 @@ function parseArgs () {
type: 'string',
describe: 'execute string as if inside `npm run-script`'
})
.option('shell-auto-fallback', {
choices: ['', 'bash', 'fish', 'zsh'],

This comment has been minimized.

@zkat

zkat May 31, 2017

Owner

Why ''?

This comment has been minimized.

@passcod

passcod May 31, 2017

Contributor

I'll re-test, but I think yargs refused to parse the option if the default wasn't in the choices. Although. Given the difficulty of figuring out the shell from a subprocess, maybe I should just remove both the autodetecting code and the default value here, and just do it based on the passed value.

This comment has been minimized.

@zkat

zkat May 31, 2017

Owner

Just don't give it a default?

This comment has been minimized.

@passcod

passcod May 31, 2017

Contributor

As to why I made the default ''... err. I don't remember. I'll have a look at the code again in its entirety to see if I can figure it out.

passcod added some commits May 31, 2017

@passcod

This comment has been minimized.

Copy link
Contributor

passcod commented May 31, 2017

Oops, I broke it. ...good reminder to add tests, I suppose. I'll do that now.
Orrrr maybe it never worked. I've confused myself. I'll figure it out.

passcod added some commits May 31, 2017

@passcod

This comment has been minimized.

Copy link
Contributor

passcod commented May 31, 2017

Ok, I've fixed it, added tests, wrote documentation. I've kept the same functionality, but I can remove the autodetect / default if you want.

@zkat

zkat approved these changes May 31, 2017

@zkat zkat merged commit ac9cb40 into zkat:latest May 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment