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

Install phantomjs locally #32

Merged
merged 5 commits into from Dec 19, 2012

Conversation

mattrobenolt
Copy link
Contributor

I don't know if there was a specific need for having phantom installed globally, but this works just great and is more explicit and inclusive.

@mattrobenolt
Copy link
Contributor Author

Actually, disregard this. It won't work after it's installed by npm because the directory structure changes up.

Any ideas on a potential solution for this? I prefer not installing modules globally, and would rather not have a library assume that it is.

@metaskills
Copy link
Collaborator

It was my understanding that as long as phantomjs is in your path, it should work. For instance, assume you install PhantomJS as a project based npm and your $PATH has something like ./node_modules/.bin in it, then this should all just work. Same goes for the mocha-phantomjs bin right?

@mattrobenolt
Copy link
Contributor Author

That is correct, but by default, ./node_modules/.bin is not on $PATH and I
don't know anyone who adds it. Convention is to use the path to the binary
automatically so you don't have to mess around with $PATH.

Sent from my iPhone

On Dec 19, 2012, at 7:46, Ken Collins notifications@github.com wrote:

It was my understanding that as long as phantomjs is in your path, it
should work. For instance, assume you install PhantomJS as a project based
npm and your $PATH has something like ./node_modules/.bin in it, then this
should all just work. Same goes for the mocha-phantomjs bin right?


Reply to this email directly or view it on
GitHubhttps://github.com//pull/32#issuecomment-11528761.

@metaskills
Copy link
Collaborator

I can hear what you are saying, but when I first started learning how to use node and npm packages, especially within projects that may have different dependencies, the internet is full of recommendations that your path should have that in it. Which mine does all the time, it has no impact on my shell otherwise and allows me to know that when I type $ cake test within the mocha-phantomjs directory, it is using the cake bin installed by npm blessed by the project and not the system cake.

@mattrobenolt
Copy link
Contributor Author

Let me come to some conclusion that works for everyone. ;)

Sent from my iPhone

On Dec 19, 2012, at 7:55, Ken Collins notifications@github.com wrote:

I can hear what you are saying, but when I first started learning how to
use node and npm packages, especially within projects that may have
different dependencies, the internet is full of recommendations that your
path should have that in it. Which mine does all the time, it has no impact
on my shell otherwise and allows me to know that when I type $ cake
testwithin the mocha-phantomjs directory, it is using the cake bin
installed by
npm blessed by the project and not the system cake.


Reply to this email directly or view it on
GitHubhttps://github.com//pull/32#issuecomment-11529007.

@metaskills
Copy link
Collaborator

Cool thanks. Right now in my head I love your point of view, but it seems to only apply for requires, not binary execution. Would love to se how this works if it can and learn a bit more.

@mattrobenolt
Copy link
Contributor Author

@metaskills Take a look at my last commit. This seems to catch everything.

If we switch to adding phantomjs as a dependency from package.json, which is how it should be in the first place, we can guarantee that it's sitting in some node_modules directory somewhere. So we can search across module.paths until we find it.

What do you think?

@@ -12,7 +12,8 @@
"scripts": {"test": "cake test"},
"dependencies": {
"mocha": ">= 1.4.x",
"commander": "~ 1.0.5"
"commander": "~ 1.0.5",
"phantomjs": "~ 0.2.3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this still work if this line was not here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe so, off hand. But I can check in a bit.

Is there a reason that you're opposed to not actually depending on
phantomjs? Seems like you'd want to declare it, but maybe I'm missing why.

Are you expecting that the end user supplies their own version of phantom
so they can choose? If so, that makes sense.

Sent from my iPhone

On Dec 19, 2012, at 9:56, Ken Collins notifications@github.com wrote:

In package.json:

@@ -12,7 +12,8 @@
"scripts": {"test": "cake test"},
"dependencies": {
"mocha": ">= 1.4.x",

  • "commander": "~ 1.0.5"
  • "commander": "~ 1.0.5",
  • "phantomjs": "~ 0.2.3"

Would this still work if this line was not here?


Reply to this email directly or view it on
GitHubhttps://github.com//pull/32/files#r2464058.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are you expecting that the end user supplies their own version of phantom so they can choose?

Yes, that is what I was thinking. The phantomjs npm does not seem to follow the same semver as PhantomJS does, so keeping it loosely coupled would help. Thanks! I'll round back to merging this later tonight too.

@mattrobenolt
Copy link
Contributor Author

Ok, this appears to be solid. It'll fetch a local phantomjs install if found first, and fall back to the global system wide phantomjs.

I don't know why the tests failed though. It seems they are a hit-or-miss in Travis?

metaskills added a commit that referenced this pull request Dec 19, 2012
@metaskills metaskills merged commit 5fad40c into nathanboktae:master Dec 19, 2012
@metaskills
Copy link
Collaborator

No, I just merged this and everything failed. Will investigate...

@metaskills
Copy link
Collaborator

OK, so I came up with a fix with 28ecde7 and just pushed out v1.1.2. Hope this helps!

@mattrobenolt
Copy link
Contributor Author

Perfect! Thanks. :)

Sent from my iPhone

On Dec 19, 2012, at 18:33, Ken Collins notifications@github.com wrote:

OK, so I came up with a fix with
28ecde728ecde7dand
just pushed out v1.1.2. Hope this helps!


Reply to this email directly or view it on
GitHubhttps://github.com//pull/32#issuecomment-11553890.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants