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

feat: Access to the various getInfo miniget requests. #901

Merged
merged 14 commits into from May 13, 2021
Merged

feat: Access to the various getInfo miniget requests. #901

merged 14 commits into from May 13, 2021

Conversation

gatecrasher777
Copy link
Contributor

In my use case it is to monitor compressed/uncompressed bandwidth, but there could be many other uses (i.e. applying a timeout or request logging)

See discussion: #899

I'm suggesting a callback which would receive each request in the getInfo pipeline, including retries.

Perhaps there is a better way... but this is my solution which is working well on my test app.

@gatecrasher777
Copy link
Contributor Author

Good points. Will revise.

@gatecrasher777
Copy link
Contributor Author

Lint does not like == wants ===. lol.

@TimeForANinja
Copy link
Collaborator

First of all thanks for contributing to this library
I wanna wait for fent to state his opinion on this thou
Besides that: Having a testcase would be cool 😅

@gatecrasher777
Copy link
Contributor Author

@TimeForANinja
Thanks for the feedback. By testcase, is that something I should provide in this PR? Is it a test using requestCallback that you want to see added to the test folder? Or is it an example using requestCallback that can be added to the example folder? I already wrote a compressed/uncompressed bandwidth example that runs the 4 test cases that are used in full-info-test.js. I would just need to merge it with this PR.

@TimeForANinja
Copy link
Collaborator

Is it a test using requestCallback that you want to see added to the test folder?

That one.
If you check the 7 succesful checks you can see, that we try to track the coverage of the module.
This helps us test the code we write.
When an Error appears for the first time we try to add a test case so that when we modify the code years later it checks that we don't bring back old bugs.
In this case it is not about previous errors but checking with every new version, that the callback is still used.

But, before you but that much work into this, i'd like to wait for fent to state his opinion.

@TimeForANinja
Copy link
Collaborator

suggested some changes in https://github.com/gatecrasher777/node-ytdl-core/pull/1 😉

@TimeForANinja TimeForANinja changed the title Access to the various getInfo miniget requests. feat: Access to the various getInfo miniget requests. May 13, 2021
@TimeForANinja TimeForANinja merged commit b2df83c into fent:master May 13, 2021
@TimeForANinja
Copy link
Collaborator

thanks again for the pr 👍

@github-actions
Copy link

🎉 This PR is included in version 4.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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