Adds support for local installation of phantomjs. Fixes #31

[mikaelbr]

I had some troubles using local installs with the latest version, as I see others have in issue #31.
This seem to be due to the pull request #29.

In CI-systems it makes sense to have local installations to not mix versions.

In my case the cmd variable (location from PhantomJS) was

somedir\node_modules\phantomjs\lib\phantom\phantomjs.exe

which caused the phantom-variable to be

somedir\node_modules\phantomjs\lib\phantom\node_modules\phantomjs\bin\phantomjs

This is a non existing path, as the path really is without the lib\phantom.

I'm not entirely sure that this is correct for all instances and all cases, but by removing the lib/phantom it worked with both global and local installs on our system - with fresh installs.

It would be great if e.g. @gillius could test this as well.

[sylvain-hamel]

Thanks a lot! I'll review it, test it and merge it tonight if its all good.

[sylvain-hamel]

@maksimr I tested you fix during lunch. It works. What do you think of this alternate implementation. I think it is easier to understand:

var win32PhantomJSPath = function () {
  // get the path stored in phantomjs\lib\location.js, someting like
  //   "C:\\Users\\user-name\\AppData\\Roaming\\npm\\phantomjs.CMD"
  var cmd = require('phantomjs').path;
  var phantom = null;

  // for a local install, we get the ".exe" for a global install we get the ".cmd"
  var isLocal = (path.extname(cmd) === ".exe");

  if (isLocal) {
    var exeLocation = path.dirname(cmd);
    var packageRoot = path.join(exeLocation, '\\..\\..');
    phantom = path.join(packageRoot, '\\bin\\phantomjs');
  } else {
    var npmGlobalRoot = path.dirname(cmd);
    phantom = path.join(npmGlobalRoot, '\\node_modules\\phantomjs\\bin\\phantomjs');
  }

  return phantom;
};

[mikaelbr]

Looks good. The .exe check looks much more robust.

I tend to be a bit more hesitant to use var-declerations inside of if-statements my self, as I think it can give a false impression of a block scope. But I don't know if it is in thread with the style of this project to manually hoist variable declarations.

[sylvain-hamel]

I agree with your comment about var-declarations inside of if-statements. So, do want to update your PR to check the exe or you prefer that I close your PR and fix it myself?

[mikaelbr]

I thought it'd be easier for me to just update the PR. Both for review reasons and less manual labor.
I've updated the code now. How does it look?

[gillius]

I ran npm install git+https://github.com/mikaelbr/karma-phantomjs-launcher.git#localInstall to get your version just now and it worked for me on my Windows 7 system where phantomjs was downloaded by this launcher (via its install.js). I don't want to test it on the second machine (XP where phantomjs is on the path) because it's our CI server doing real builds and I have to do a commit to our real code to get it there :).

[sylvain-hamel]

@mikaelbr Looks good to me! Would you please squash your 3 commits before I merge this in? Thanks a lot.

[mikaelbr]

Done and done! No problem!

[sylvain-hamel]

@mikaelbr I have one last request for you. Would you please change your commit message to:

fix: add support for local installation of phantomjs on windows
Fixes #31

We need this to generate our changelog correctly.

[deepak1556]

@sylvain-hamel can this block be replaced with just

return path.join(isLocal ? process.cwd() : absolutePath, '/node_modules/phantomjs/bin/phantomjs');

[sylvain-hamel]

@deepak1556

• The forward slashes make it a lot easier to read
• process.cwd() : is this reliable? could external code change it before we get here?
• I like to see the npmGlobalRoot and packageRoot variables though. It helps understand how we come up with those paths

[deepak1556]

• hmm the slashes dont matter as join takes care of it for us
• yup cwd() is reliable for a process
• well in that case assign packageRoot to just process.cwd().

[sylvain-hamel]

• i know, that's why I prefer / to \\, just like you did
• but could't another plug-in change cwd before we get here?
• indeed

@mikaelbr would you please consider those comments when you send updated PR (for the commit message)?

[mikaelbr]

• I very much agree with using /.
• Using process.cwd() will force using phantomjs installed as a depencency to this runner? What if phantomjs is installed as a project dep. it self? require('phantomjs').path can return path to a dependency outside of this module. Is this a problem at all?

[mikaelbr]

I've updated to use / instead of \\, and the commit message.
I'm still sceptical towards using process.cwd() and not basing local installs on PhantomJS' location. But I can update this if you really want.

[deepak1556]

cool then! , sry i didnt think about getting a phantom binary from outside the module.

[sylvain-hamel]

@mikaelbr did you see alternate fix #33? What do you think about it?

[mikaelbr]

For me it comes down to the two main differences: Either run PhantomJS through node bin or use the executable directly. I don't feel I have enough knowledge about PhantomJS it self to say if running the exe directly it self is bad or not, but it seems like you'd miss out on the error handling and such from the bin file?

[sylvain-hamel]

Hi @mikaelbr, I merged #33, it is closer to what other launchers are do. Thanks a lot for the effort you have put into this and for your feedback. I truly appreciate!