-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
chore: use grunt to optionally install the dependency #3996
Conversation
package.json
Outdated
"sinon": "^6.1.4", | ||
"sinon-chai": "^3.2.0", | ||
"strong-error-handler": "^3.0.0", | ||
"strong-task-emitter": "^0.0.8", | ||
"supertest": "^3.0.0" | ||
}, | ||
"optionalDependencies": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please not that optionalDependencies
are installed in production, e.g. when an app depends on loopback. With your change in place, every LoopBack application is going to install phantomjs! That would be a poor user experience IMO.
We need to mark phantomjs-prebuilt
as an optional dev-dependency instead.
Unfortunately, npm
does not support optional dev-dependencies out of the box, at least as far as I can tell from searching the internet and the discussion in npm/npm#3870.
A possible workaround is offered by this npm package https://www.npmjs.com/package/optional-dev-dependency
Considering that we have a single optional dev-dependency only, it may be simpler to simply execute npm install --no-save phantomjs-prebuilt
from our Gruntfile. (Maybe check if the package has not been already installed, to speed up repeated execution of npm t
for framework developers practicing test-driven development.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 and good to know a way to install dependencies optionally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely 👏
connected to #3993
@slnode test please |
connected to #3993
Description
Related issues
Checklist
guide