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

Use the phantomjs-prebuilt module #70

Closed
wants to merge 2 commits into from

Conversation

gwynjudd
Copy link
Contributor

Resolves #53 and fixes the path to phantomjs mentioned in #66 in a better way

@dflynn15
Copy link
Owner

While I understand the desire to have Phantom as a part of the plugin I have kept away from including it for two main reasons.

First, it keeps this project tied specifically to a version of Phantom JS which I really do not wish to do. Tying the project down to specific versions will then lead to people asking for the option for other versions, etc. which in the end is not the focus of this plugin. I want people to be able to choose what they want with Phantom (version and all). If you want to get a beta version, go for it! If you want to use a deprecated 0.x sure, have fun! Ultimately, I want people to have options. Plus, it prevents anyone from having to potentially download yet another version of phantom because of versioning (i.e. grunticon and others).

Secondly, by expecting it globally and only looking for it in node_modules afterwards it gives the us the opportunity to not pull down a heavy npm package. There are many use cases where having long standing npm install scripts can crash servers, cost development time, and really just frustrate us all. In allowing the developer to "choose their own path" we can allow people to not have to include Phantom as a npm package but have it globally installed (faster npm install!!).

I'll open up another PR that allows for checking for the .cmd file like it previously did and we can go from there. Thanks so much for all your help on this!

@gwynjudd
Copy link
Contributor Author

OK no worries

@gwynjudd gwynjudd deleted the phantom-prebuilt branch June 26, 2016 22:25
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.

Error on Windows10(x64)
2 participants