Skip to content
This repository has been archived by the owner on Jul 24, 2019. It is now read-only.

Adds methods: exec() and run() #588

Merged
merged 3 commits into from
Aug 3, 2016
Merged

Adds methods: exec() and run() #588

merged 3 commits into from
Aug 3, 2016

Conversation

cognitom
Copy link
Contributor

@cognitom cognitom commented Aug 2, 2016

  • exec() is just a shortcut of childProcess.spawn()
  • run() returns a promise, and waits until PhantomJS get ready

To use phantomjs-prebuilt with WebDriver, we needed to detect when PhantomJS gets ready. But that was little bit messy, like this:

new Promise((resolve, reject) => {
  const program = spawn(phantomjs.path, ['--webdriver=4444'])
  let isFirst = true
  program.stdout.on('data', data => {
    // This detects PhantomJS instance get ready.
    if (!isFirst) return
    isFirst = false
    resolve(program)
  })
}).then(program => {
  // do something
  program.kill()
})

By this PR, it'll be easy and readable:

phantomjs.run('--webdriver=4444').then(program => {
  // do something
  program.kill()
})

If you like this PR, I'd like to add some README text, too. Thanks.

- exec() is just a shortcut of childProcess.spawn()
- run() returns a promise, and waits until PhantomJS get ready
@nicks
Copy link
Contributor

nicks commented Aug 3, 2016

cool! i like this!

can you fill out a CLA real quick?
https://github.com/Medium/opensource/blob/master/sign-cla.md

* @returns {Promise} the process of PhantomJS
*/
exports.run = function () {
var program = exports.exec.apply(null, arguments)
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens right now if you pass bogus arguments to the binary? does it resolve or just hang?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, for example, --bogus makes it hang (without error messages). Is it better to be guarded like this?
http://phantomjs.org/api/command-line.html

Copy link
Contributor

Choose a reason for hiding this comment

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

oh, i was just imagining that we'd reject the promise if the process existed before the promise resolved.

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, meant to say "if the process exited before the promise resolved". if it's not easy to do, don't worry about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see. I'll have a look. Wait a bit. Thanks.

@cognitom
Copy link
Contributor Author

cognitom commented Aug 3, 2016

@nicks sure! Will do.

@cognitom
Copy link
Contributor Author

cognitom commented Aug 3, 2016

@nicks the logic is wrapped by try block now 😄
Could you have a look?

Updated: README.md, too.

@nicks
Copy link
Contributor

nicks commented Aug 3, 2016

thanks!

@nicks nicks merged commit e649b58 into Medium:master Aug 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants