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

Optional dependencies #682

Merged
merged 10 commits into from
Oct 17, 2013
Merged

Optional dependencies #682

merged 10 commits into from
Oct 17, 2013

Conversation

Turbo87
Copy link
Contributor

@Turbo87 Turbo87 commented Oct 15, 2013

The request library has a rather large dependency chain that not everybody needs. In cases with very little disk space (e.g. embedded platforms) this can be a problem. This pull request changes some of these dependencies into optional dependencies, which are still installed by default, but are skipped when installed via npm install --no-optional. The relevant unit tests are skipped in case the modules are not found on the system.

@mikeal
Copy link
Member

mikeal commented Oct 15, 2013

this is already planned for the 3.0 branch. as this is a breaking change it'll need to be held off until 3.0.

@mikeal
Copy link
Member

mikeal commented Oct 15, 2013

oh wait, i didn't realize the default still installs these.

give me a day or so to consider this patch, i'd like some more feedback.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Oct 15, 2013

yes, exactly. the default behavior is just like before, unless you install the library with --no-optional.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Oct 15, 2013

rebased the commits on top of the current master to take advantage of Travis CI checking

@Turbo87
Copy link
Contributor Author

Turbo87 commented Oct 15, 2013

as you can see in https://api.travis-ci.org/jobs/12582058/log.txt?deansi=true only the required dependencies are getting installed when using npm install --no-optional, while https://api.travis-ci.org/jobs/12582056/log.txt?deansi=true shows the default npm install.

@Turbo87
Copy link
Contributor Author

Turbo87 commented Oct 17, 2013

@mikeal please let me know if you have any more questions about this

mikeal added a commit that referenced this pull request Oct 17, 2013
@mikeal mikeal merged commit 034e84e into request:master Oct 17, 2013
@gabrielf
Copy link

I know this has already been fixed but 👍

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.

None yet

3 participants