Alert if ytdl-core is old #779

redbrain · 2020-11-16T23:28:07Z

From my experience in the YTDL support server, upgrading the package to latest seems to be something many users forget to do, and they encounter errors because of it.

If merged, this PR would add an alert in the console if ytdl-core is outdated, along with instructions to update.

This requires no extra dependencies and has been made as minimal as possible so as not to add overhead.
To function, it relies on the GitHub API.

To maintain this feature, all that is required is to update the tag ('v4.0.5') in the if statement with each new release.
This was written in 5 minutes, so please make edits if desired.

By the way, thank you for keeping this library up-to-date; it's quite a gift.

Peppy8651 · 2020-11-16T23:38:03Z

Nice. If someone would merge this that would be great.

Peppy8651 · 2020-11-16T23:38:40Z

I'll test this out.

redbrain · 2020-11-16T23:52:25Z

Looks like the linter hates me, but that's because code written to be small is naturally difficult to read.
Here's the code passed through the de-uglifier. If you'd like to use that instead, make the change; you have permission to do so.

miniget("https://api.github.com/repos/fent/node-ytdl-core/releases/latest", {
    headers: {
        "User-Agent": "a/b"
    }
}).text().then(response => {
    if ("v4.0.5" !== JSON.parse(response).tag_name) {
        console.log("\x1B[31mWARNING:\x1B[0m ytdl-core is out of date! Update it using 'npm i ytdl-core'.");
    }
});

Peppy8651 · 2020-11-17T00:21:06Z

Yea I have Eslint too. I will make some changes so my linter doesn't give me an error or something.

Peppy8651 · 2020-11-17T00:28:58Z

Just finished changes that will probably fit with the linter.

Peppy8651 · 2020-11-17T00:33:51Z

These are the new changes I made. However, I get an error.
ytdl-core.zip
Unhandled promise rejection: TypeError: Cannot read property 'player_response' of undefined at pipeline (C:\Users\Owner\Documents\BirdyBirdy\node_modules\ytdl-core\lib\info.js:126:19) at runMicrotasks (<anonymous>) at processTicksAndRejections (internal/process/task_queues.js:93:5) at async exports.getBasicInfo (C:\Users\Owner\Documents\BirdyBirdy\node_modules\ytdl-core\lib\info.js:39:14) at async Object.execute (C:\Users\Owner\Documents\BirdyBirdy\commands\play.js:41:14)

...hopefully. why is readability so important in a line that serves no more than utility?

I'd really like to see all the checks pass for once. This code is absolutely readable, I dare you to say otherwise.

redbrain · 2020-11-17T01:17:08Z

After being unable to read the eslintrc for 2 minutes, I realised that lint will always fail since "no-console" is set.
fent'll have to give this the okay then, because console.logging is not okay with the computer.

take a look at the first commit for the original code, and the second one for more readable code.
3 and 4 were trying to figure out why the linter failed.

fent

hi! this is a good idea, i've left some comments.

additionally

could use some sort of rate limiting. maybe a local file that stores the last time the version was checked, and only check once a day at most. but i'd feel better about an in memory variable due to the point below
could be moved into the getInfo functions, in case an app runs for a long time, they'd be able to check for a new version whenever they call getInfo(), rate limited of course.
should have an option to disable this. i've seen a few occasions of people using an older version for "reasons". i woudn't recommend it, but you can't stop someone from using the program how they like. maybe checkUpdates/Release or skip(Update/Release)Check?

fent · 2020-11-17T01:18:37Z

lib/index.js

@@ -8,6 +8,14 @@ const miniget = require('miniget');
 const m3u8stream = require('m3u8stream');
 const parseTime = require('m3u8stream/dist/parse-time');

+const current = 'v4.0.5';
+const message = '\x1B[31mWARNING:\x1B[0m ytdl-core is out of date! Update it using "npm i ytdl-core".';
+miniget('https://api.github.com/repos/fent/node-ytdl-core/releases/latest', { headers: { 'User-Agent': 'a/b' } })


is the User-Agent header needed?

while I was testing this, I got 403s from the service if it wasn't there. weird, but any valid user-agent fixed it.
Using node-fetch seems to solve that entirely, but i didnt want to add another dependency.

lib/index.js

As suggested by fent Co-authored-by: fent <fentbox@gmail.com>

lib/index.js

fent · 2020-11-17T01:59:18Z

should have an option to disable this. i've seen a few occasions of people using an older version for "reasons". i woudn't recommend it, but you can't stop someone from using the program how they like. maybe checkUpdates/Release or skip(Update/Release)Check?

or maybe and environment variable to more easily disable for tests

lgtm-com · 2020-11-17T03:03:01Z

This pull request introduces 1 alert when merging 801a47a into fcfbc8e - view on LGTM.com

new alerts:

1 for Unused variable, import, function or class

redbrain

The things you have requested should now be done in patch-1. Let me know if there's more you'd like to see or something I've missed. Feel free to change anything obvious.

env var UPDATE if not set: check twice daily settings: 'always', 'never'

lib/info.js

Specifically: change env var to YTDL_NO_UPDATE which takes true or false Drop code outside of functions (that's what I think you meant by that request. will it still run during long-uptime?) Clearer message, calc ms number. gonna drop the env definition into nock, hold on

I'm migrating to a new computer so I'm doing this all from GH web, it's a nightmare

I fixed this already, huh whuh? Useful commit comment goes here

redbrain · 2020-11-17T13:03:15Z

Would someone mind fixing the code formatting according to eslint?
I'm on a different device so I'm using GH web and I have no idea what the linter is demanding of me now.

Peppy8651 · 2020-11-17T15:27:17Z

Ok, here's some changes to the current one that I made so it fits with my linter. Don't know how Github will handle it though.
ytdl-core.zip

If not, please fix it for me.

lib/info.js

Fixed the indents for eslint (hopefully, it seems to dislike me) Implemented request of not checking until the first 12 hrs are over Made it a function, and called it inside the getInfo cache thing directly above it. Also exported it so it could be used programatically (seems like a good idea?) Please lmk if there's anything else you'd like to see.

subtitle: my brain is big and smart coming to theaters near y'all

fent · 2020-11-18T01:17:34Z

lib/info.js

+let lastUpdateCheck = Date.now();
+const checkForUpdates = () => {
+  if (!process.env.YTDL_NO_UPDATE && Date.now() - lastUpdateCheck >= 1000 * 60 * 60 * 12) {
+    miniget('https://api.github.com/repos/fent/node-ytdl-core/releases/latest', { headers: { 'User-Agent': 'a/b' } })


i was looking at github's docs for their api
https://developer.github.com/v3/#user-agent-required

let's put "ytdl-core" here for the User-Agent. they're hosting the repo after all.

this fails the linter bc the line's too long. what do you suggest i do?

add line breaks between the options

..., { headers: { 'User-Agent': 'ytdl-core' }, })

redbrain · 2020-11-18T04:34:53Z

what is codecov, and is the error something i can resolve?

fent · 2020-11-18T05:39:48Z

what is codecov, and is the error something i can resolve?

it's for code coverage, as in is the how you introduced tested. if you're up for it, you can add tests. otherwise, i can do it after the merge.

and there are some eslint warnings again

redbrain · 2020-11-18T18:25:59Z

Lint's throwing comma-dangle on the console.warn line, and I have no clue why. I'm assuming the rest of the errors stem from that.
As for testing, I'm not knowledgeable for writing tests, so I'd rather you work on that.
Do you know why the linter is screaming again? I can't figure that out.

fent · 2020-11-18T19:20:42Z

you can run npm run lint:fix it should fix those lint errors for you

fent · 2020-11-24T05:38:11Z

going to merge this as is, then correct the lint errors and add tests. thanks @redbrain! looking forward to the next update

meoyawn · 2020-11-25T12:30:43Z

shouldn't it be part of https://github.com/fent/node-ytdl and not the core? People use core as a library in their projects

redbrain · 2020-11-26T17:43:53Z

The goal is to alert library users. (it will alert node-ytdl users as well since it relies on the library)

meoyawn · 2020-11-26T18:28:13Z

gotcha 😞

I'd use npm or yarn for that

and there's https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/ and other bots

redbrain · 2020-11-27T15:55:52Z

and there's https://github.blog/2020-06-01-keep-all-your-packages-up-to-date-with-dependabot/ and other bots

See the first message in this discussion for why this was implemented:

upgrading the package to latest seems to be something many users forget to do, and they encounter errors because of it.

stoically · 2021-01-01T15:24:22Z

lib/info.js

@@ -426,6 +426,21 @@ const getM3U8 = async(url, options) => {
  return formats;
 };

+// Check for updates.
+const { version } = require('../package.json');


This breaks when bundling with webpack, not sure why. It imports the package.json from my project root, instead of ytdl-core's package.json. Workaround for me was to add a "version" field to my package.json, so it doesn't error out with

TypeError: pkg.version is undefined

Might very well be me configuring webpack incorrectly, but thought I'd mention it here.

Repo to reproduce the issue: https://github.com/stoically/humanetube/

weird. i'd ask webpack about it. otherwise, not sure what's causing webpack to import the wrong package.json

node's require works a bit different for relative paths, it looks for files relatively to the file it is required from.

I bundled it with Browserify and it worked fine; I don't know enough about Webpack to help you further.

see: fent/node-ytdl-core#779 (review)

Alert if ytdl-core is old

5f77450

redbrain added 3 commits November 16, 2020 19:55

Add variables and pacify the linter

f1adbfd

...hopefully. why is readability so important in a line that serves no more than utility?

Cause the linter to not be angry, again

4a1ba92

I'd really like to see all the checks pass for once. This code is absolutely readable, I dare you to say otherwise.

Change console.log to console.error for the linter

bacb248

fent requested changes Nov 17, 2020

View reviewed changes

Get version from package.json

801a47a

As suggested by fent Co-authored-by: fent <fentbox@gmail.com>

fent reviewed Nov 17, 2020

View reviewed changes

lib/index.js Outdated Show resolved Hide resolved

redbrain commented Nov 17, 2020

View reviewed changes

redbrain added 3 commits November 16, 2020 23:11

Allow console.warn

7ebd26a

Move the warn code to getInfo()

cc8b41b

Add env variable support

520bea3

env var UPDATE if not set: check twice daily settings: 'always', 'never'

fent requested changes Nov 17, 2020

View reviewed changes

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

redbrain added 5 commits November 17, 2020 07:54

Add env definition as requested

86dfbbc

Fix the eslintrc

1e7e8ca

Indent properly

49dc1e4

I'm migrating to a new computer so I'm doing this all from GH web, it's a nightmare

Appease the linter once more

86ad00b

I fixed this already, huh whuh? Useful commit comment goes here

Maybe this will be correct indentation?

b30b77a

If not, please fix it for me.

fent requested changes Nov 17, 2020

View reviewed changes

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

lib/info.js Outdated Show resolved Hide resolved

redbrain added 2 commits November 17, 2020 19:52

Fixing silly mistakes

efe9c4b

subtitle: my brain is big and smart coming to theaters near y'all

fent reviewed Nov 18, 2020

View reviewed changes

so that's why it wanted a user-agent

3bedace

Make line shorter for the linter

240143e

Peppy8651 mentioned this pull request Nov 18, 2020

Warn the user in their console if their DJS version is outdated. discordjs/discord.js#5020

Closed

2 tasks

fent merged commit 5cfc0eb into fent:master Nov 24, 2020

stoically reviewed Jan 1, 2021

View reviewed changes

stoically added a commit to stoically/humanetube that referenced this pull request Jan 1, 2021

fix: workaround for ytdl-core's package.json import

4054e2b

see: fent/node-ytdl-core#779 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alert if ytdl-core is old #779

Alert if ytdl-core is old #779

redbrain commented Nov 16, 2020

Peppy8651 commented Nov 16, 2020

Peppy8651 commented Nov 16, 2020

redbrain commented Nov 16, 2020 •

edited

Peppy8651 commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

redbrain commented Nov 17, 2020

fent left a comment

fent Nov 17, 2020

redbrain Nov 17, 2020

fent commented Nov 17, 2020

lgtm-com bot commented Nov 17, 2020

redbrain left a comment

redbrain commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

fent Nov 18, 2020

redbrain Nov 18, 2020

fent Nov 18, 2020

redbrain commented Nov 18, 2020

fent commented Nov 18, 2020

redbrain commented Nov 18, 2020

fent commented Nov 18, 2020

fent commented Nov 24, 2020

meoyawn commented Nov 25, 2020

redbrain commented Nov 26, 2020

meoyawn commented Nov 26, 2020 •

edited

redbrain commented Nov 27, 2020

stoically Jan 1, 2021

fent Jan 2, 2021

redbrain Jan 2, 2021

Alert if ytdl-core is old #779

Alert if ytdl-core is old #779

Conversation

redbrain commented Nov 16, 2020

Peppy8651 commented Nov 16, 2020

Peppy8651 commented Nov 16, 2020

redbrain commented Nov 16, 2020 • edited

Peppy8651 commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

redbrain commented Nov 17, 2020

fent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fent commented Nov 17, 2020

lgtm-com bot commented Nov 17, 2020

redbrain left a comment

Choose a reason for hiding this comment

redbrain commented Nov 17, 2020

Peppy8651 commented Nov 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbrain commented Nov 18, 2020

fent commented Nov 18, 2020

redbrain commented Nov 18, 2020

fent commented Nov 18, 2020

fent commented Nov 24, 2020

meoyawn commented Nov 25, 2020

redbrain commented Nov 26, 2020

meoyawn commented Nov 26, 2020 • edited

redbrain commented Nov 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redbrain commented Nov 16, 2020 •

edited

meoyawn commented Nov 26, 2020 •

edited