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

Add phantomjs-prebuilt to dependency list #123

Merged
merged 1 commit into from Jun 23, 2016

Conversation

joelmukuthu
Copy link
Contributor

In order to have it automatically installed as a peer dependency in
npm@3 as well as in npm@2.

Closes #104

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@joelmukuthu
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@joelmukuthu
Copy link
Contributor Author

Ping @dignifiedquire / @zzo. From #104 it seems that more and more people are experiencing this issue, is this an acceptable solution to you?

@zzo
Copy link
Contributor

zzo commented Jun 16, 2016

i think you can take it out of peerDeps and devDeps if it's blatantly in the deps list. But this solution seems legit.
Also update the commit message to match https://karma-runner.github.io/0.13/dev/git-commit-msg.html

@joelmukuthu
Copy link
Contributor Author

joelmukuthu commented Jun 16, 2016

I would have argued to keep it in peerDependencies (for the sake of npm2 users) but since the desired end result is to have a phantomjs binary in ./node_modules/.bin/, I agree with that solution. I will update my commit and force-push.

@CarmenPopoviciu
Copy link

anything I can help with to have this merged in? Would gladly help out ^_^

@joelmukuthu joelmukuthu changed the title Added phantomjs-prebuilt to dependency list Add phantomjs-prebuilt to dependency list Jun 23, 2016
@zzo zzo merged commit 215bf0e into karma-runner:master Jun 23, 2016
@zzo
Copy link
Contributor

zzo commented Jun 23, 2016

v1.0.1 released to npm with this

@CarmenPopoviciu
Copy link

CarmenPopoviciu commented Jun 23, 2016

@zzo you sir are awesome! Thank you for all the great work you are doing. And thank you @joelmukuthu for fixing the issue.

bugbug

@dignifiedquire
Copy link
Member

@zzo why did you merge this? This forces everyone using phantom 1 to install phantom 2 even if they don't need it. Before you simply installed the correct version side by side (given npm@3) and for npm@2 it would install it together.

@zzo
Copy link
Contributor

zzo commented Jun 23, 2016

without phantom this fails and disk is cheap :)
Seriously maybe this should have been a bigger version jump but if they don't want phantom/know what they are doing they can pin to the previous version. perhaps this instead could be a post-install hook, if phantom is not present then install it.

@dignifiedquire
Copy link
Member

Disk space yes, bandwith often is slow and expensive :(

@CarmenPopoviciu
Copy link

I dug a bit deeper into the repo history, because I wanted to better understand your concerns, and found the issue that moved phantomjs to peerDependencies. I also checked the npm GH discussion related to the changed behaviour of peerDependencies for npm3, which helped me understand much better the intention behind their move and what the expected course of actions in case of conflicts is.

Apparently the solution for #104 was indeed installing phantomjs-prebuilt manually, as per npm peerDependencies conflict management. Things probably broke because the module was renamed and since it's explicitly required by the code(which made me wonder if this is indeed a good fit for peerDeps), that made any previous local phantom installations incompatible.

josephfrazier added a commit to josephfrazier/browser-extension that referenced this pull request Jul 14, 2016
It was renamed to phantomjs-prebuilt, and karma-phantomjs-launcher now
installs it. See the following for details:

Medium/phantomjs#447 (reference)
karma-runner/karma-phantomjs-launcher#123
josephfrazier added a commit to OctoLinker/OctoLinker that referenced this pull request Jul 14, 2016
I'm hoping this will fix the intermittent error described in #115 (comment)

* Upgrade phantomjs to ^2.1.7

* Upgrade karma-html2js-preprocessor to ^1.0.0

* Upgrade karma-mocha to ^1.1.1

* Upgrade karma-mocha-reporter to ^2.0.4

* Add missing peer dependency karma

* Upgrade karma-phantomjs-launcher to ^1.0.1

* Remove phantomjs from devDependencies

It was renamed to phantomjs-prebuilt, and karma-phantomjs-launcher now
installs it. See the following for details:

Medium/phantomjs#447 (reference)
karma-runner/karma-phantomjs-launcher#123
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants