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

Don't pass bnr flags to specified command #70

Merged
merged 1 commit into from Jul 31, 2017
Merged

Conversation

laverdet
Copy link
Contributor

@laverdet laverdet commented Jun 25, 2017

Currently bnr-specific flags are forwarded to the ran command which seems like a pretty bad idea.

Consider package.json:

{
  "scripts": {
    "hello": "bnr hello -s"
  },
  "betterScripts": {
    "hello": {
      "command": "node hello.js"
    }
  },
  "dependencies": {
    "better-npm-run": "0.0.15"
  }
}

And hello.js is just console.log(process.argv).

Running npm run hello will output:
[ 'node', 'hello.js', '-s' ]

This is undesirable because if the command you are running also has some kind of meaning assigned to these flags then you can't use bnr.

This pull request allows you to put bnr flags before the specified command. Those arguments will be consumed by bnr and then the command only gets arguments which appeared after. This shouldn't break existing applications because the behavior remains the same for the old way. I've updated the README with the preferred way of passing flags.

If you don't mind breaking existing applications (major semvar update), then a more correct fix would be to have bnr ignore the flags after the specified command and just forward them. Perhaps you can do that some time in the future after landing this patch.

@galenandrew
Copy link
Collaborator

@laverdet thank you for your PR!! This change makes sense to me and looks like it will resolve #68.

@benoror thoughts?

Copy link
Owner

@benoror benoror left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@galenandrew
Copy link
Collaborator

@benoror this looks like it would be a breaking change since it's changing the order of arguments in users' package.json. Thoughts on a major version bump – v1.0.0?

@laverdet
Copy link
Contributor Author

laverdet commented Jul 7, 2017

It doesn't break existing invocations, actually. If the paramters appear after the invoked script they are passed forward and interpreted by bnr (matches current behavior). If they are passed before the script they are consumed and not passed forward. Before the patch, flags before the script would be an error.

I mentioned in the original comment that a more a correct solution would be to not interpret flags after the script.. but that would be a breaking change which seemed to aggressive for a random pull request.

@galenandrew
Copy link
Collaborator

@laverdet thanks for the clarification…you're right.

@benoror benoror merged commit 2e5a0cc into benoror:master Jul 31, 2017
@benoror
Copy link
Owner

benoror commented Jul 31, 2017

@laverdet thanks!

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.

None yet

3 participants