search

Medium / phantomjs

NPM wrapper for installing phantomjs
Other
1.43k stars 435 forks source link

fix issue with tryPhantomjsInLib on Elastic Beanstalk #590

Closed thoop closed 8 years ago

thoop commented 8 years ago

In v2.1.7, tryPhantomjsInLib wouldn't overwrite location.js if phantomjs was found

In v2.1.8, tryPhantomjsInLib would overwrite location.jswith an absolute location if phantomjs was found

This breaks Elastic Beanstalk because they put your app in /tmp/deployment, run npm install and npm rebuild, then move your app to /var/app/current.

The npm rebuild was causing location.js to have an absolute path as the location, which was no longer valid once the app was moved to /var/app/current instead of /tmp/deployment.

The change I made should preserve some of the code cleanup by conditionally overwriting location.js if findValidPhantomJsBinary returns a valid path.

Please bump the patch version after this is merged since it's a breaking change affecting all current Elastic Beanstalk deploys :)

Thanks for your work on this repo!

thoop commented 8 years ago

Forgot to mention the issue relating to this PR:

https://github.com/Medium/phantomjs/issues/583

nicks commented 8 years ago

thanks for diagnosing this! would you mind filling out a CLA real quick? https://github.com/Medium/opensource/blob/master/sign-cla.md

thoop commented 8 years ago

Signed the CLA

thoop commented 8 years ago

@nicks mind merging this and bumping the version when you get a chance? Thanks so much!

nicks commented 8 years ago

sure, it may take me a little bit to get to it though