Skip to content
This repository has been archived by the owner on Jan 9, 2024. It is now read-only.

I made the code compatible with the latest data return #88

Merged
merged 8 commits into from
Nov 19, 2020

Conversation

dbrn
Copy link
Contributor

@dbrn dbrn commented Nov 15, 2020

Some things still need to be tested throughout. But it works for my app and I think this could be a good starting point to make the library work again.

Some things still need to be tested throughout. But it works for my app and I think this could be a good starting point to make the library work again.
@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2020

This pull request introduces 2 alerts when merging b4971ac into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands
  • 1 for Missing variable declaration

@dbrn
Copy link
Contributor Author

dbrn commented Nov 15, 2020

I see there are some problems with the linter, could someone please fix them?

@lgtm-com
Copy link

lgtm-com bot commented Nov 15, 2020

This pull request introduces 1 alert when merging 4012769 into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands

@mirsella
Copy link

mirsella commented Nov 15, 2020

thanks for helping them fix it.

for me it isn't working :

const ytpl = require('ytpl')
ytpl("playlist_url")
  .then(playlist => console.log(playlist))

i downloaded your fix with npm i https://github.com/dbrn/node-ytpl#patch-1

i got 2 different error with 2 differents playlists.

with this playlist : https://www.youtube.com/playlist?list=PL2788304DC59DBEB4
I'm getting :

/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:104
  const scriptBodyObject = JSON.parse(scriptBody);
                                ^

SyntaxError: Unexpected token ; in JSON at position 155360
    at JSON.parse (<anonymous>)
    at getDataObject (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:104:33)
    at Object.exports.getGeneralInfo (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:26:22)
    at module.exports (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/firstpage.js:29:25)
    at processTicksAndRejections (node:internal/process/task_queues:93:5)
    at async module.exports (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/main.js:13:20)

and with this playlist : https://www.youtube.com/playlist?list=PLnYA0n5BTNscRlnFBkNGJrCyKdOqGtID9
I'm getting :

/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:96
  string.replace(/\n\r?/g, ' ')
         ^

TypeError: Cannot read property 'replace' of undefined
    at exports.removeHtml (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:96:10)
    at Object.exports.getGeneralInfo (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/util.js:42:18)
    at module.exports (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/firstpage.js:29:25)
    at processTicksAndRejections (node:internal/process/task_queues:93:5)
    at async module.exports (/home/mirsella/Downloads/ytpl/node_modules/ytpl/lib/main.js:13:20)

thanks you. have a goodnight

Fixed a bug that prevented the correct parsing of some JSON objects.
Added a try-catch in the removeHtml function.
@dbrn
Copy link
Contributor Author

dbrn commented Nov 16, 2020

Thank you for your feedback and patience, @mirsella!
I fixed those two bugs, and now it seems to work fine.
Check the latest commit!

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 1 alert when merging 0906021 into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands

@lgtm-com
Copy link

lgtm-com bot commented Nov 16, 2020

This pull request introduces 1 alert when merging 4a9ec78 into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands

@mirsella
Copy link

work for me 👍

@dealoux
Copy link

dealoux commented Nov 16, 2020

Thank you for your fix

Unfortunately I'm getting an error

    at Object.exports.buildVideoObject (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\util.js:100:62)
    at P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:23
    at Array.map (<anonymous>)
    at module.exports (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:6)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
    at async module.exports (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\main.js:13:20)
    at async PlayCommand.run (P:\Software Projects\Discord Bot\Aqukin\src\opus\commands\play.js:50:13)

I think it has something to do with obtaining the playlist/track duration, thank you.

@dbrn
Copy link
Contributor Author

dbrn commented Nov 16, 2020

Can I see the error message, please?

@dealoux
Copy link

dealoux commented Nov 16, 2020

I tried it again with different playlists and the error seems to occur only when there're private/removed videos in the playlist. If not then it seems to be working fine, thank you.

@dbrn
Copy link
Contributor Author

dbrn commented Nov 16, 2020

I tried it again with different playlists and the error seems to occur only when there're private/removed videos in the playlist. If not then it seems to be working fine, thank you.

If you still have the log of the previous error, please send it over :) I'd love to give it a look!

@dealoux
Copy link

dealoux commented Nov 16, 2020

I tried it again with different playlists and the error seems to occur only when there're private/removed videos in the playlist. If not then it seems to be working fine, thank you.

If you still have the log of the previous error, please send it over :) I'd love to give it a look!

I'm testing it with my bot using this function

ytpl(query, { limit: Infinity }).then(async playlist =>{
   console.log(playlist);
   // do something
}).catch((err) => {
    console.log(err);
    // do something 
)});

I'm getting the error while trying with this playlist, or any playlist with at least one private/removed video actually: https://www.youtube.com/playlist?list=PLpt61bADOMwXIZpLr09sCNeocN7CXjcoS

The only thing that it logged is the error TypeError: Cannot read property 'simpleText' of undefined at ytpl\lib\util.js:100:62

TypeError: Cannot read property 'simpleText' of undefined
    at Object.exports.buildVideoObject (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\util.js:100:62)
    at P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:23
    at Array.map (<anonymous>)
    at module.exports (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:6)

This playlist is working without the error: https://www.youtube.com/playlist?list=PL4o29bINVT4EG_y-k5jGoOu3-Am8Nvi10

I'm pretty certain that the error is originated from trying to obtain the info of the private video, thank you

@dbrn
Copy link
Contributor Author

dbrn commented Nov 17, 2020

@dealoux it should be fine now :)

@lgtm-com
Copy link

lgtm-com bot commented Nov 17, 2020

This pull request introduces 1 alert when merging e088e06 into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands

@dealoux
Copy link

dealoux commented Nov 17, 2020

Thank you @dbrn for your fix, I tested it with the mentioned function and there's still an error with any playlist with more than 100 videos or requires more than a page. Playlists with less than 100 videos or within the 1st page are working correctly.

From what I know the Youtube returns the playlist in pages at 100 videos each, hence I think it has to do with the way the playlist pages are obtained rather than your fix being the problem.

It's working correctly with these two playlists
https://www.youtube.com/playlist?list=PLDIoUOhQQPlXr63I_vwF9GD8sAKh77dWU (Exactly 100 videos)
https://www.youtube.com/playlist?list=PLpt61bADOMwXIZpLr09sCNeocN7CXjcoS (35 videos including 2 private ones)

These two playlist result in the error TypeError: Cannot read property 'shortBylineText' of undefined
https://www.youtube.com/playlist?list=PLx0sYbCqOb8QTF1DCJVfQrtWknZFzuoAE (101 videos including 1 private)
https://www.youtube.com/playlist?list=PL4o29bINVT4EG_y-k5jGoOu3-Am8Nvi10 (240 videos)

TypeError: Cannot read property 'shortBylineText' of undefined
    at Object.exports.buildVideoObject (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\util.js:95:46)
    at P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:23
    at Array.map (<anonymous>)
    at module.exports (P:\Software Projects\Discord Bot\Aqukin\node_modules\ytpl\lib\firstpage.js:33:6)

I really appreciate your efforts 👍

@dbrn
Copy link
Contributor Author

dbrn commented Nov 18, 2020

Thank you @dealoux!
I'm trying to deliver something that could make this library work again... And without your feedback, I wouldn't be able to do it :)
Let me see what I can do for what you've just told me.

It was preventing to correctly show some files that didn't feature a short description
@dbrn
Copy link
Contributor Author

dbrn commented Nov 18, 2020

Now the lib shouldn't crash anymore.
Hopefully, it works with longer lists as well. Here it seems to work fine.

@lgtm-com
Copy link

lgtm-com bot commented Nov 18, 2020

This pull request introduces 1 alert when merging 070d9cb into 63716c5 - view on LGTM.com

new alerts:

  • 1 for Identical operands

@dealoux
Copy link

dealoux commented Nov 18, 2020

Thank you @dbrn, it works for me now.
I'm glad that I can help, I'll be sure to let you know if there's any problem left. Please have a good day/night 👍

@TimeForANinja TimeForANinja changed the base branch from master to wip-api-adjustments November 19, 2020 10:04
@TimeForANinja
Copy link
Owner

TimeForANinja commented Nov 19, 2020

added another wip-branch and changed the merge target
i'll have a look later today and most likely merge it as a starting point
so thanks for the work already 👍

Edit: the linter failing is no problem since tests will crash anyway

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging 070d9cb into 15ef16c - view on LGTM.com

new alerts:

  • 1 for Identical operands

@dbrn
Copy link
Contributor Author

dbrn commented Nov 19, 2020

added another wip-branch and changed the merge target
i'll have a look later today and most likely merge it as a starting point
so thanks for the work already 👍

Edit: the linter failing is no problem since tests will crash anyway

Thank you.
I'm here if you need further help.

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging b5d31c2 into 15ef16c - view on LGTM.com

new alerts:

  • 1 for Identical operands

@TimeForANinja TimeForANinja merged commit 791f759 into TimeForANinja:wip-api-adjustments Nov 19, 2020
@TimeForANinja
Copy link
Owner

😉 watch out with using linters
after fixing the eslint run it wasn't happy

@lgtm-com
Copy link

lgtm-com bot commented Nov 19, 2020

This pull request introduces 1 alert when merging c937c4f into 15ef16c - view on LGTM.com

new alerts:

  • 1 for Identical operands

@dbrn
Copy link
Contributor Author

dbrn commented Nov 19, 2020

Thank you 😅

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants