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

expose dowloadFromInfo and drop some dependencies #1

Merged
merged 2 commits into from
Jul 3, 2014
Merged

expose dowloadFromInfo and drop some dependencies #1

merged 2 commits into from
Jul 3, 2014

Conversation

andrewrk
Copy link
Contributor

This pull request:

  • retains backward compatibility
  • removes request caching. Instead you are supposed to use getInfo followed by downloadFromInfo.
  • drops dependency on eventvat
  • drops dependency on streamify, uses built-in PassThrough stream instead.

All tests pass.

@fent
Copy link
Owner

fent commented Jun 30, 2014

I always thought that the download function returning a stream was a much easier to use and understand API. What are your reasons for preferring the callback version?

I agree with getting rid of removing EventVat.

@andrewrk
Copy link
Contributor Author

I don't really care either way - stream vs callback. This just seemed like an easier way to implement downloadFromInfo. Actually we can probably implement the backwards-compatible stream API on top of this, let me try that.

@andrewrk andrewrk changed the title idea for a simpler API / drop some dependencies expose dowloadFromInfo and drop some dependencies Jun 30, 2014
@andrewrk
Copy link
Contributor Author

OK I updated the pull request. Now it does not break backwards compatibility but it still removes 2 dependencies and exposes downloadFromInfo.

var request = require('request');
var _ = require('underscore');
var getInfo = require('./info');
var crequest = require('./crequest');
var util = require('./util');

module.exports = ytdl;
ytdl.getInfo = getInfo;
ytdl.downloadFromInfo = downloadFromInfo;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add docs to readme for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@andrewrk
Copy link
Contributor Author

andrewrk commented Jul 3, 2014

@fent I made downloadFromInfo a streaming API, same as the ytdl API. See what you think!

stream.resolve(req);
var format = util.chooseFormat(info.formats, options);
if (format instanceof Error) {
// give user a chance to attach 'error' handler
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full sentence with period please.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@fent
Copy link
Owner

fent commented Jul 3, 2014

Looks good. Awesome.

Thank you for putting up with the back and forth, and contributing.

👍

fent added a commit that referenced this pull request Jul 3, 2014
expose dowloadFromInfo and drop some dependencies
@fent fent merged commit bdd9a40 into fent:master Jul 3, 2014
@andrewrk
Copy link
Contributor Author

andrewrk commented Jul 3, 2014

And thank you for having an open mind to these changes :)

azrafe7 pushed a commit to azrafe7/node-ytdl-core that referenced this pull request Feb 14, 2018
expose dowloadFromInfo and drop some dependencies
TimeForANinja added a commit that referenced this pull request Mar 5, 2022
TimeForANinja added a commit that referenced this pull request Mar 5, 2022
* chore(package): update miniget to version 4.2.2

* drop node v10 & update package-lock.json

* update workflow versions

* try #1 to fix unit tests

Co-authored-by: depfu[bot] <23717796+depfu[bot]@users.noreply.github.com>
Co-authored-by: TimeForANinja <[email protected]>
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.

2 participants