Skip to content
This repository has been archived by the owner on Sep 7, 2022. It is now read-only.

Adds support for local installation of phantomjs. Fixes #31 #32

Closed
wants to merge 1 commit into from
+8 −6
Closed

Adds support for local installation of phantomjs. Fixes #31 #32

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,16 @@ 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 absolutePath = path.dirname(cmd);

// get the global npm install directory by removing the filename from cmd variable
var npmGlobalRoot = path.dirname(cmd);

// add known path
var phantom = npmGlobalRoot + '\\node_modules\\phantomjs\\bin\\phantomjs';
// for a local install, we get the ".exe" for a global install we get the ".cmd"
var packageRoot, isLocal = (path.extname(cmd) === ".exe");
if (isLocal) {
packageRoot = path.join(absolutePath, '/../..');
return path.join(packageRoot, '/bin/phantomjs');
}
Copy link

Choose a reason for hiding this comment

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

@sylvain-hamel can this block be replaced with just

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link

Choose a reason for hiding this comment

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

  • 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().

Copy link
Contributor

Choose a reason for hiding this comment

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

  • 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)?

Copy link
Author

Choose a reason for hiding this comment

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

  • 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?


return phantom;
return path.join(absolutePath, '/node_modules/phantomjs/bin/phantomjs');
};

var PhantomJSBrowser = function(baseBrowserDecorator, config, args) {
Expand Down