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

Fix running as child process #29

Merged
merged 1 commit into from
Jul 8, 2016
Merged

Fix running as child process #29

merged 1 commit into from
Jul 8, 2016

Conversation

nkbt
Copy link
Collaborator

@nkbt nkbt commented Jul 8, 2016

What:
When running as child process with arguments (like --config smth), process.argv is never empty, though commander has no args.

Also it would be logically more correct to check program.args, since later we use it as program.args[0].

/Users/nkbt/nkbt/react-swap/node_modules/p-s/dist/bin-utils.js:90
    scripts = program.args[0].split(',');
                             ^

TypeError: Cannot read property 'split' of undefined
    at getScriptsAndArgs (/Users/nkbt/nkbt/react-swap/node_modules/p-s/dist/bin-utils.js:90:30)
    at loadAndRun (/Users/nkbt/nkbt/react-swap/node_modules/p-s/dist/bin/p-s.js:47:56)
    at Object.<anonymous> (/Users/nkbt/nkbt/react-swap/node_modules/p-s/dist/bin/p-s.js:43:3)

Why

I want to proxy p-s from my component like

require('child_process')
  .spawn(ps, ['--config', config].concat(process.argv.slice(2)), {
    cwd: process.cwd(),
    env: process.env,
    stdio: [process.stdin, process.stdout, process.stderr]
  });

To avoid adding extra dependency.

@kentcdodds
Copy link
Collaborator

This makes sense and as long as the build passes I say :shipit: :-)

@codecov-io
Copy link

codecov-io commented Jul 8, 2016

Current coverage is 100%

Merging #29 into master will not change coverage

@@           master   #29   diff @@
===================================
  Files           8     8          
  Lines         187   187          
  Methods         0     0          
  Messages        0     0          
  Branches        0     0          
===================================
  Hits          187   187          
  Misses          0     0          
  Partials        0     0          

Powered by Codecov. Last updated by 367f274...06ede51

@kentcdodds kentcdodds merged commit 636df9b into sezna:master Jul 8, 2016
@nkbt nkbt deleted the fix-running-as-child-process branch July 8, 2016 10:43
@nkbt
Copy link
Collaborator Author

nkbt commented Jul 8, 2016

Cheers 🍻

@kentcdodds
Copy link
Collaborator

FYI, this broke the following case:

p-s -p lint,test

Because program.args is an empty array in this case. I'm working on a fix now.

kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
@kentcdodds
Copy link
Collaborator

Also @nkbt, as a thanks for the contribution, I've given you write access to this repo. Please continue to make pull requests as you see fit :-)

kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
kentcdodds pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
tleunen pushed a commit that referenced this pull request Jul 8, 2016
PR #29 broke the following case:

```
p-s -p lint,test
```

This commit fixes it by getting the scripts from the args first and
seeing if there are any scripts available to run. If not, then we show
the help message.
@nkbt
Copy link
Collaborator Author

nkbt commented Jul 9, 2016

Oh wow, sorry for the break, I don't have this case =(... Prob we need some e2e tests

@kentcdodds
Copy link
Collaborator

I should set up dont-break with this project on travis. Do you have any suggestions for projects I can do that with?

@kentcdodds
Copy link
Collaborator

The problem I see with using dont-break is that it could be that the p-s project is working fine, but the dependency's tests are broken. I think just regular e2e tests would be good. Maybe we could use nixt.

@nkbt
Copy link
Collaborator Author

nkbt commented Jul 9, 2016

I would go for just a shell script for CI/local =). Bash for the win. something like

npm start blah > test.log
cat test.log | grep "all good"

I have bunch of this sort of e2e tests for our cli tools and docker images.

@kentcdodds
Copy link
Collaborator

Created #31 for anyone to take ownership 👍

@nkbt
Copy link
Collaborator Author

nkbt commented Jul 9, 2016

I'll get some basic stuff done since I have bunch of little scripts ready
to use.
On Sun, 10 Jul 2016 at 07:01, Kent C. Dodds [email protected]
wrote:

Created #31 #31 for anyone to
take ownership 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAKsoBbGLuQgSUtVFxE656JgnQIxe7Zmks5qUAwpgaJpZM4JH59v
.

@nkbt
Copy link
Collaborator Author

nkbt commented Jul 9, 2016

Nixt looks pretty sick by the way. Ill give it a try here
On Sun, 10 Jul 2016 at 07:02, Nikita Butenko [email protected] wrote:

I'll get some basic stuff done since I have bunch of little scripts ready
to use.
On Sun, 10 Jul 2016 at 07:01, Kent C. Dodds [email protected]
wrote:

Created #31 #31 for anyone to
take ownership 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#29 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAKsoBbGLuQgSUtVFxE656JgnQIxe7Zmks5qUAwpgaJpZM4JH59v
.

@kentcdodds
Copy link
Collaborator

Sweet! 👍

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