Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for npm3 or dedupe #41

Closed
Frizi opened this issue Jul 27, 2015 · 6 comments
Closed

Add support for npm3 or dedupe #41

Frizi opened this issue Jul 27, 2015 · 6 comments
Assignees
Labels

Comments

@Frizi
Copy link

Frizi commented Jul 27, 2015

Right now phantomjs bin path is hardcoded here
https://github.com/localnerve/html-snapshots/blob/master/lib/html-snapshots.js#L23

When html-snapshots is installed with npm3 (or dedupe was used), phantom is installed in different location. In my case this path would work '../../.bin/phantomjs', but you cannot assume that really.

You may try your luck with this code from karma-phantomjs-launcher:

var phantomJSExePath = function () {
  // If the path we're given by phantomjs is to a .cmd, it is pointing to a global copy. 
  // Using the cmd as the process to execute causes problems cleaning up the processes 
  // so we walk from the cmd to the phantomjs.exe and use that instead.

  var phantomSource = require('phantomjs').path;

  if (path.extname(phantomSource).toLowerCase() === '.cmd') {
    return path.join(path.dirname( phantomSource ), '//node_modules//phantomjs//lib//phantom//phantomjs.exe');
  }

  return phantomSource;
};
@localnerve
Copy link
Owner

This has always been an issue - well before npm3.
The hard-coded path you reference is just a default.

The default bin path can be overriden with the phantomjs option, found at the bottom of this section in the readme.
So, the phantomjs supplied with this lib is entirely optional.

I'm assuming you are suggesting that there should be a different default phantomjs location. I've a couple of questions on that implied suggestion:

  1. What phantomjs location & version are you actively referencing that prompted this issue (presume via this lib pkg json)?
    I ask this because last year we found a package driven MacOSX difference that invalidated referencing phantomjs/lib/phantom/phantomjs.exe directly (perms on file not executable after install). Maybe this has changed again and would be good to know which version to get to test a next change.
    Also, could be that you are getting a different phantomjs in your particular project dep situation.
  2. Is npm3 changing files/folders?
  3. Is the package referenced in karma-phantomjs-launcher using the same phantomjs package referenced by this lib?

Thanks in advance for any additional feedback you can supply.

@Frizi
Copy link
Author

Frizi commented Jul 28, 2015

  1. paths to all 3 modules:
    • node_modules/html-snapshots, v0.8.1, dependency on phantomjs ^1.9.16
    • node_modules/karma-phantomjs-launcher, v0.1.4, dependency on phantomjs ~1.9
    • node_modules/phantomjs, v1.9.17
  2. packages are now installed as flat as possible, per-package node_modules folder is only created when there is a version conflict.
    https://github.com/npm/npm/releases/tag/v3.0.0 check section "Flat, flat, flat!"
  3. Yes, at least this lib should use it, but doesn't properly.

Simple require('phantomjs') will always refer to the correct path, so it should be a preferred solution to find a module.

@localnerve
Copy link
Owner

Let's be more specific on what you are saying the change should be (So I'm certain I understand). Here's my attempt:

Update the default phantomjs path value used by this package to be dependent on the path property exported from the phantomjs npm package. The default value will now come from this code in the phantomjs package.

Using this property should always be the correct path for all npm versions, and will serve well in the npm3, dedupe case.

Easy enough, please confirm I got it right.
Thanks!

@Frizi
Copy link
Author

Frizi commented Aug 7, 2015

Yes, you got it right. The code I posted at the top is actually working well in all these cases. Just remember to allow the user to alter this path anyway.

@localnerve
Copy link
Owner

OK, I have a release candidate that runs correctly on Linux. It also makes successful substitutions via the phantomjs option (this is how I run on Travis-CI).

I noticed that this technique (you posted from above, the essence of the change) is only used by karma-phantomjs-launcher for win32.
I'm using it for every/any platform. Works for Linux.

Can you please review (c2f5e28) the change and test your use case? Also, let me know your OS. I'll be spinning up a Mac test shortly.

One way to test: Drop the RC in your package.json via git://github.com/localnerve/html-snapshots#rc-0.10.0
(do npm update, not install after that tweak)

@localnerve
Copy link
Owner

I've confirmed working on mac, any feedback in your use case?

@localnerve localnerve self-assigned this Oct 25, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants