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

Added support for task arguments #43

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

stevenvachon
Copy link

@stevenvachon stevenvachon commented Jun 23, 2017

This is waiting on #46


This change is Reviewable

@M-Zuber
Copy link
Owner

M-Zuber commented Jun 26, 2017

Overall this looks good, but I found the following edge cases that would still need to work:

  • npm run watch should simply run all tasks defined
  • npm run watch -- ...args should run all tasks defined passing in the args to each one

here is the setup I was using to test it
package.json:

{
    "name": "npm-watch test",
    "version": "1",
    "devDependencies": {
        "npm-watch": "file:path to local version of npm-watch"
    },
    "scripts": {
        "watch": "npm-watch",
        "echo": "node index.js",
        "echo2": "node foo.js"
    },
    "watch": {
        "echo": "index.js",
        "echo2": "foo.js"
    }
}

index.js

function foo(arg) {
    console.log(arguments);
}
foo(process.argv.slice(2));

foo.js

function foo(arg) {
    console.log(arguments);
}
foo(process.argv.slice(2).reverse());

@M-Zuber M-Zuber self-requested a review June 26, 2017 13:53
@M-Zuber M-Zuber self-assigned this Jun 26, 2017
@stevenvachon
Copy link
Author

We should add a test suite.

@M-Zuber
Copy link
Owner

M-Zuber commented Jun 27, 2017

We should add a test suite.

That is something I would love to do, if you have an idea on how to implement, please open an issue/pr so we can discuss it in a dedicated location

@stevenvachon
Copy link
Author

I'd like to do it here in a separate commit, as it'll help me be sure that my PR passes your above case.

@M-Zuber
Copy link
Owner

M-Zuber commented Jun 27, 2017

Generally I would prefer to separate something like that out to a dedicated PR, but as long as each commit is self contained, go ahead and do it on this PR

…with a more reusable API that can facilitate tests.
…when task name is specified. It is not currently possible to support arguments without a task because argv[2] was hard-coded to be the task name. If it were defined as “--arg”, it would treat that as the task name.

fixes M-Zuber#37
…by only exporting what is necessary for production use.
@M-Zuber
Copy link
Owner

M-Zuber commented Jun 28, 2017

Travis and AV are both connected now

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

Successfully merging this pull request may close these issues.

2 participants