search

peerigon / phridge

A bridge between node and PhantomJS
The Unlicense
519 stars 50 forks source link

Close webpage on call page dispose #20

Closed Mikxail closed 9 years ago

Mikxail commented 9 years ago

Webpage must be closed on dispose.

coveralls commented 9 years ago

Coverage Status

Coverage decreased (-0.45%) when pulling 7eae1814a78dc26cb214645571fceaff3e7805b2 on Mikxail:master into 68958c7ab991e663456451fa2269dfbf51961736 on peerigon:master.

jhnns commented 9 years ago

Sorry, this pull-request contains two different features and should be split.

I'm ok with providing phantomJsOptions (since the config doesn't seem to work with some values #19), but there should be only one way to do so. Either via command arguments or via config-file.

What's more: Have you tested your fix? I don't think that command arguments will work without the ---prefix. You should include tests for that.

Mikxail commented 9 years ago

Sorry for bad pull request. I'll split it and do pull request again.

What way you prefer for providing options to phantomjs?

  1. Doesn't use config file and use only run options. CamelCase parametres can be converted to dash style for back compatible.
  2. spawn function can receive 2 arguments. First argument is ignored if second argument will be passed
  3. spawn function can receive only 1 argument. Pass arguments to run options if it is array/string. Pass to config file If it is object

I think, 1st is the best solution. But full back compatible may not worked.

jhnns commented 9 years ago

Yep, the first one is the best. There are also great modules on npm to turn camelCase to dash-case.