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

Move optionalDependencies to dependencies. #2720

Conversation

fengmk2
Copy link

@fengmk2 fengmk2 commented Oct 28, 2015

Like source-map is not the optional dependency on Node.js,
should use dependencies instead of optionalDependencies.

Like `source-map` is not the optional dependency on Node.js,
should use `dependencies` instead of `optionalDependencies`.
@fengmk2
Copy link
Author

fengmk2 commented Oct 28, 2015

see npm/npm#2737

optionalDependencies should be optional, event npm install fail, it don't broken the module.

@SomMeri
Copy link
Member

SomMeri commented Nov 30, 2015

@fengmk2 Thank you for taking the time to make the pull request.

Unfortunately, one of AppVeyor tests failed and we can not merge it in when they are failing. The change is failing on node 4.2.1. The other problem is that not all optionalDependencies are non-optional. For example, image-size should not be needed unless you use image-size less.js function.

I would merge in change that moves only really non-optional entries like source-map into mandatory dependencies and does not fail unit tests.

@fengmk2
Copy link
Author

fengmk2 commented Nov 30, 2015

@SomMeri Nice!

@matthew-dean
Copy link
Member

Is this PR getting updated?

@stale
Copy link

stale bot commented Nov 14, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 14, 2017
@stale stale bot closed this Nov 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants