fix: Switched from launching phantomjs via node to ensuring that the .exe is always launched #33
Conversation
| @@ -48,12 +47,7 @@ var PhantomJSBrowser = function(baseBrowserDecorator, config, args) { | |||
| optionsCode.join('\n') + '\npage.open("' + url + '");\n'; | |||
| fs.writeFileSync(captureFile, captureCode); | |||
|
|
|||
| var isWin = /^win/.test(process.platform); | |||
| if (isWin) { | |||
| flags = flags.concat(win32PhantomJSPath(), captureFile); | |||
There was a problem hiding this comment.
The extra flag added here caused the command to be
> node path/to/phantomjs /path/to/capture.js
But when PHANTOMJS_BIN was set, it resulted in a command similar to:
> global/path/to/phantomjs.exe path/to/phantomjs /path/to/capture.js
Which caused phantomjs to hang.
|
This Pull Request provide an alternative approach to fixing the issues tackled in #32 This approach allows us to continue treating phantomjs as a browser executable and with one small extra bit of code to 'fix up' the cmd path to be the exe path on windows. The approach I'd rather see is the cleanup code doing a recursive process kill on the process it launched, but I don't have the node expertise, code familiarity, or cross platform knowledge to pull that off. |
|
@vojtajina, @sylvain-hamel We're being forced to fix / work around this issue because even though our package.json is locked at I've no problem contributing back and I'm happy to try and help, but it was I was a bit shocked to see our CI builds fail, when we felt we had locked all our dependencies. |
|
@chad-configit Thank a lot for your help! I like this alternative approach. |
|
@chad-configit does #32 have any drawbacks in your context or is this just a matter of choosing the cleanest solution? |
A bit of both. Because you can override the phantomjs_bin path using env, that meant if you did that it would try to run the phantomjs script thru phantomjs.exe, not node.exe. This was fixable but led to more codepaths and I prefered to par things back to a simpler solution. Additionally after considering that, it seemed much nicer for phantomjs to be being called in the same way as chrome or any other 'normal browser', which seemed desirable from a code maintenance / understandability point of view. |
|
@chad-configit would you please reword your commit message to something that reads well as a one-liner in a changelog using at the present tense. Something like this maybe? This will speed up the merge process. |
|
@sylvain-hamel Done. I couldn't think of a better message 😄
No worries, it's understandable. Package management can be a pain. |
…in32-process-launching fix : add support for phantomjs installed locally or with PHANTOMJS_BIN
|
@chad-configit I merged you PR. Thanks a lot! |
ghost
deleted the
No description provided.