Phantom 1.7 fix #51

Closed
wants to merge 12 commits into
from

Conversation

@amir20
Owner

amir20 commented Sep 25, 2012

This is an option pull request that merges two other pull request to fix bugs. I am using this for my own projects.

baudehlo and others added some commits Jul 24, 2012

Fix deprecation warnings.
- Use 'express()' instead of 'express.createServer()'
- Use `path.existsSync` instead of `fs.existsSync`
- Remove trailing whitespaces
- Add travis CI.
@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Sep 25, 2012

Owner

Should also fix #50.

Owner

amir20 commented Sep 25, 2012

Should also fix #50.

@kent

This comment has been minimized.

Show comment Hide comment
@kent

kent Oct 5, 2012

@amir20 until this gets pulled, how can I use it in my app?

kent commented Oct 5, 2012

@amir20 until this gets pulled, how can I use it in my app?

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Oct 5, 2012

Owner

@kent You can update your package.json file to my branch.

"phantom":"git://github.com/amir20/phantomjs-node.git#phantom-1.7-fix"
Owner

amir20 commented Oct 5, 2012

@kent You can update your package.json file to my branch.

"phantom":"git://github.com/amir20/phantomjs-node.git#phantom-1.7-fix"
@kent

This comment has been minimized.

Show comment Hide comment
@kent

kent Oct 5, 2012

Thanks!

kent commented Oct 5, 2012

Thanks!

@juriejan

This comment has been minimized.

Show comment Hide comment
@juriejan

juriejan Oct 11, 2012

Contributor

I've worked from this branch to add argument passing when evaluating statements if you're at all interested.

https://github.com/edgecampus/phantomjs-node/tree/edgecampus

Contributor

juriejan commented Oct 11, 2012

I've worked from this branch to add argument passing when evaluating statements if you're at all interested.

https://github.com/edgecampus/phantomjs-node/tree/edgecampus

@sheebz

This comment has been minimized.

Show comment Hide comment
@sheebz

sheebz Oct 12, 2012

Frustrating that this issue has been open for so long despite the pull requests. I had to write my own implementation to get this. If you really need this in a node ap, check out https://npmjs.org/package/phantom-proxy

sheebz commented Oct 12, 2012

Frustrating that this issue has been open for so long despite the pull requests. I had to write my own implementation to get this. If you really need this in a node ap, check out https://npmjs.org/package/phantom-proxy

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Oct 12, 2012

Owner

@juriejan I can merge your pull request in to mine also. I have been keeping up with this.

Owner

amir20 commented Oct 12, 2012

@juriejan I can merge your pull request in to mine also. I have been keeping up with this.

@juriejan

This comment has been minimized.

Show comment Hide comment
@juriejan

juriejan Oct 12, 2012

Contributor

Cool. Let me know if you need anything from me. I'm adding small things here and there. Check the messages when deciding what you'd like to add.

Contributor

juriejan commented Oct 12, 2012

Cool. Let me know if you need anything from me. I'm adding small things here and there. Check the messages when deciding what you'd like to add.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Oct 15, 2012

Owner

@juriejan I merged this with my branch. You should be able to point to my branch and get all the changes that I have merged and yours.

Owner

amir20 commented Oct 15, 2012

@juriejan I merged this with my branch. You should be able to point to my branch and get all the changes that I have merged and yours.

@toranb

This comment has been minimized.

Show comment Hide comment
@toranb

toranb Nov 18, 2012

Any reason this pull request has not yet been merged into master?

toranb commented Nov 18, 2012

Any reason this pull request has not yet been merged into master?

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Nov 18, 2012

Owner

I don't think the owner is maintaining anymore.

Owner

amir20 commented Nov 18, 2012

I don't think the owner is maintaining anymore.

@toranb

This comment has been minimized.

Show comment Hide comment
@toranb

toranb Nov 18, 2012

If that's the case I vote we have the npm module switched to someone who is willing to maintain it under the name "phantom" sgentle#57

toranb commented Nov 18, 2012

If that's the case I vote we have the npm module switched to someone who is willing to maintain it under the name "phantom" sgentle#57

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Nov 18, 2012

Owner

I agree. Right now I have been doing this on my own branch. I might just re-release it under another name.

Owner

amir20 commented Nov 18, 2012

I agree. Right now I have been doing this on my own branch. I might just re-release it under another name.

@jokeyrhyme

This comment has been minimized.

Show comment Hide comment
@jokeyrhyme

jokeyrhyme Nov 18, 2012

It's a shame because @sgentle definitely seems to be alive and active according to https://github.com/sgentle?tab=activity

In the meantime, I've had some success using https://github.com/sheebz/phantom-proxy instead, although it doesn't seem to be as deterministic as this project (that might be my fault though).

It's a shame because @sgentle definitely seems to be alive and active according to https://github.com/sgentle?tab=activity

In the meantime, I've had some success using https://github.com/sheebz/phantom-proxy instead, although it doesn't seem to be as deterministic as this project (that might be my fault though).

@OllieJennings

This comment has been minimized.

Show comment Hide comment
@OllieJennings

OllieJennings Nov 21, 2012

still having problems when using this 1.7 fix relating to issue #31 on ubuntu 12.04

still having problems when using this 1.7 fix relating to issue #31 on ubuntu 12.04

@asaaki

This comment has been minimized.

Show comment Hide comment
@asaaki

asaaki Dec 11, 2012

Thanks to @amir20 - this helped a lot!

asaaki commented Dec 11, 2012

Thanks to @amir20 - this helped a lot!

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Dec 11, 2012

Owner

@asaaki You are welcome!

Owner

amir20 commented Dec 11, 2012

@asaaki You are welcome!

@jescalan

This comment has been minimized.

Show comment Hide comment
@jescalan

jescalan Dec 11, 2012

Thank you - this pull request saved my project! 😄 - please merge this if you have a chance @sgentle

Thank you - this pull request saved my project! 😄 - please merge this if you have a chance @sgentle

@jokeyrhyme

This comment has been minimized.

Show comment Hide comment
@jokeyrhyme

jokeyrhyme Dec 11, 2012

@amir20 Note, if you check "Files changed", you've accidentally (I assume) versioned the node_modules directory containing upstream modules and dependencies.

.gitignore should be updated if necessary, and the node_modules directory should be removed from the repository. This makes it difficult to see exactly what this Pull Request is actually changing.

@amir20 Note, if you check "Files changed", you've accidentally (I assume) versioned the node_modules directory containing upstream modules and dependencies.

.gitignore should be updated if necessary, and the node_modules directory should be removed from the repository. This makes it difficult to see exactly what this Pull Request is actually changing.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Dec 12, 2012

Owner

@jokeyrhyme Yea I noticed that too this morning. I will do a push to fix it.

Owner

amir20 commented Dec 12, 2012

@jokeyrhyme Yea I noticed that too this morning. I will do a push to fix it.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Dec 12, 2012

Owner

@jokeyrhyme Fixed now.

Owner

amir20 commented Dec 12, 2012

@jokeyrhyme Fixed now.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Dec 12, 2012

Owner

Just a note to all that at this point the coffee script is not in sync with the js anymore. I tried doing make build and the new js file does not work. I am not sure what is going on.

Owner

amir20 commented Dec 12, 2012

Just a note to all that at this point the coffee script is not in sync with the js anymore. I tried doing make build and the new js file does not work. I am not sure what is going on.

@bikedorkjon

This comment has been minimized.

Show comment Hide comment
@bikedorkjon

bikedorkjon Dec 19, 2012

"You can update your package.json file to my branch."

I'm just getting started with node and haven't done this before, can you explain what I need to do a bit more? Thanks.

"You can update your package.json file to my branch."

I'm just getting started with node and haven't done this before, can you explain what I need to do a bit more? Thanks.

@jatinder85

This comment has been minimized.

Show comment Hide comment
@jatinder85

jatinder85 Dec 19, 2012

Thank you this pull worked smoothly.

Thank you this pull worked smoothly.

@ehartford

This comment has been minimized.

Show comment Hide comment
@ehartford

ehartford Jan 4, 2013

Is there a fork that is published to npm? @sgentle

Is there a fork that is published to npm? @sgentle

@kn0ll

This comment has been minimized.

Show comment Hide comment
@kn0ll

kn0ll Jan 4, 2013

@amir20 firstly, thanks for your work. secondly, i too have been having syncing problems with js vs coffee (in sgentle's repository, not your fork). in some cases i do a fresh npm install and get an old version of shim.js which does not match what is in the repository. on other machines, i get the latest. ironically, when the machine gets an outdated version of shim.js it works seemingly fine with Phantom 1.8. unfortunately, i can't make this happen consistently, so now i am looking to your branch. i realize this is a strange issue.

that said, unfortunately i am receiving an error with your fork:

/var/www/apps/getglue-static-server/node_modules/phantom/node_modules/dnode/node_modules/socket.io/lib/manager.js:470
    if (this.roomClients[id][name]) {
                            ^
TypeError: Cannot read property '' of undefined

have you seen this before? it's very possible it's something specific to my implementation: i am running many phantom processes across many child processes. as such, i actually had to submit a pull request of my own to make it work. i was only able to test your branch by merging it with my own. after merging, i recompiled phantom.coffee.

did you make changes to phantom.js and not phantom.coffee that my compilation would have overrode? or, have you ever seen this error before? i realize this could have arisen from merging both our changes so if i am wasting your time i apologize. :)

kn0ll commented Jan 4, 2013

@amir20 firstly, thanks for your work. secondly, i too have been having syncing problems with js vs coffee (in sgentle's repository, not your fork). in some cases i do a fresh npm install and get an old version of shim.js which does not match what is in the repository. on other machines, i get the latest. ironically, when the machine gets an outdated version of shim.js it works seemingly fine with Phantom 1.8. unfortunately, i can't make this happen consistently, so now i am looking to your branch. i realize this is a strange issue.

that said, unfortunately i am receiving an error with your fork:

/var/www/apps/getglue-static-server/node_modules/phantom/node_modules/dnode/node_modules/socket.io/lib/manager.js:470
    if (this.roomClients[id][name]) {
                            ^
TypeError: Cannot read property '' of undefined

have you seen this before? it's very possible it's something specific to my implementation: i am running many phantom processes across many child processes. as such, i actually had to submit a pull request of my own to make it work. i was only able to test your branch by merging it with my own. after merging, i recompiled phantom.coffee.

did you make changes to phantom.js and not phantom.coffee that my compilation would have overrode? or, have you ever seen this error before? i realize this could have arisen from merging both our changes so if i am wasting your time i apologize. :)

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Jan 4, 2013

Owner

@ericstob No it is not published. I created #1 and will plan on deploying it to npm. For now use my branch.

@catshirt I have never seen that error. Honestly, I just started merging people's pull requests because @sgentle didn't.

Owner

amir20 commented Jan 4, 2013

@ericstob No it is not published. I created #1 and will plan on deploying it to npm. For now use my branch.

@catshirt I have never seen that error. Honestly, I just started merging people's pull requests because @sgentle didn't.

@akhil

This comment has been minimized.

Show comment Hide comment
@akhil

akhil Jan 25, 2013

Who is maintaining the module now? Where can i get one with all the fixes?

akhil commented Jan 25, 2013

Who is maintaining the module now? Where can i get one with all the fixes?

@ehartford

This comment has been minimized.

Show comment Hide comment
@ehartford

ehartford Jan 25, 2013

I used phantoms webdriver interface instead, that bypassed all these
issues.
On Jan 25, 2013 1:14 PM, "Akhil Kodali" notifications@github.com wrote:

Who is maintaining the module now? Where can i get one with all the fixes?


Reply to this email directly or view it on GitHubhttps://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12721178.

I used phantoms webdriver interface instead, that bypassed all these
issues.
On Jan 25, 2013 1:14 PM, "Akhil Kodali" notifications@github.com wrote:

Who is maintaining the module now? Where can i get one with all the fixes?


Reply to this email directly or view it on GitHubhttps://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12721178.

@akhil

This comment has been minimized.

Show comment Hide comment
@akhil

akhil Jan 28, 2013

Which component/package did you use?

On Fri, Jan 25, 2013 at 4:40 PM, Eric Stob notifications@github.com wrote:

I used phantoms webdriver interface instead, that bypassed all these
issues.
On Jan 25, 2013 1:14 PM, "Akhil Kodali" notifications@github.com wrote:

Who is maintaining the module now? Where can i get one with all the
fixes?


Reply to this email directly or view it on GitHub<
https://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12721178>.


Reply to this email directly or view it on GitHubhttps://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12722180.

akhil commented Jan 28, 2013

Which component/package did you use?

On Fri, Jan 25, 2013 at 4:40 PM, Eric Stob notifications@github.com wrote:

I used phantoms webdriver interface instead, that bypassed all these
issues.
On Jan 25, 2013 1:14 PM, "Akhil Kodali" notifications@github.com wrote:

Who is maintaining the module now? Where can i get one with all the
fixes?


Reply to this email directly or view it on GitHub<
https://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12721178>.


Reply to this email directly or view it on GitHubhttps://github.com/sgentle/phantomjs-node/pull/51#issuecomment-12722180.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Jan 28, 2013

Owner

@akhil you can use the one from my branch. I should have time soon to push to npm. For now just use the branch.

Owner

amir20 commented Jan 28, 2013

@akhil you can use the one from my branch. I should have time soon to push to npm. For now just use the branch.

@amir20 amir20 closed this Jan 30, 2013

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Jan 30, 2013

Owner

Hello everybody, sorry for the delay. I have deployed my version of this module to npm. The new name is phantom-v2. I am plan to use it for my app later today to make sure it works. I am more than happy to take pull requests and merge them. Hopefully this helps every body. The repo is located https://github.com/amir20/phantomjs-node-v2. You should be able to do npm install phantom-v2 to get this.

Owner

amir20 commented Jan 30, 2013

Hello everybody, sorry for the delay. I have deployed my version of this module to npm. The new name is phantom-v2. I am plan to use it for my app later today to make sure it works. I am more than happy to take pull requests and merge them. Hopefully this helps every body. The repo is located https://github.com/amir20/phantomjs-node-v2. You should be able to do npm install phantom-v2 to get this.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Jan 30, 2013

Owner

I have confirmed that this works with latest version of node 1.8.1.

Owner

amir20 commented Jan 30, 2013

I have confirmed that this works with latest version of node 1.8.1.

@reverie

This comment has been minimized.

Show comment Hide comment
@reverie

reverie Feb 16, 2013

https://github.com/amir20/phantomjs-node-v2 doesn't exist, nor does 'phantom-v2' in npm. I assume they were merged into the offical phantom package and the official repo.

reverie commented Feb 16, 2013

https://github.com/amir20/phantomjs-node-v2 doesn't exist, nor does 'phantom-v2' in npm. I assume they were merged into the offical phantom package and the official repo.

@amir20

This comment has been minimized.

Show comment Hide comment
@amir20

amir20 Feb 16, 2013

Owner

@reverie yes it has been merged to this repo and deployed to npm.

Owner

amir20 commented Feb 16, 2013

@reverie yes it has been merged to this repo and deployed to npm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment