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

Add twitch support #1017

Merged
merged 6 commits into from
Apr 6, 2023
Merged

Add twitch support #1017

merged 6 commits into from
Apr 6, 2023

Conversation

riccardobl
Copy link
Contributor

Search for ln address in channel bio.
Test channel: https://www.twitch.tv/rblb0/

@riccardobl
Copy link
Contributor Author

I added some code to cache the twitch client id, since it should never change. However, since it might change, the code tries to fetch the resource twice, once with the cached client id, and if it fails, again with a newly obtained client id that is then cached.

@escapedcat escapedcat requested review from bumi and reneaaron June 10, 2022 05:53
@riccardobl riccardobl mentioned this pull request Jun 10, 2022
19 tasks
@fczuardi
Copy link
Contributor

fczuardi commented Jul 28, 2022

@riccardobl On your test channel, https://www.twitch.tv/rblb0/, there is a ⚡ emoji on the contents of the page because you don't have recent streams, I used a different test page here https://www.twitch.tv/sedentarismo where the user only see the lightning address if navigating to the "About" page. Do you know if there is some setting that the content creator can use to always display the lightning address on the main channel page?

@fczuardi
Copy link
Contributor

Looking at the source code of the page with devtools, there is a <meta name="twitter:description" content="⚡rblb@getalby.com"> under the <head> tag. I wonder if we could use that instead of the proposed API fetch

@riccardobl
Copy link
Contributor Author

Looking at the source code of the page with devtools, there is a <meta name="twitter:description" content="⚡rblb@getalby.com"> under the <head> tag. I wonder if we could use that instead of the proposed API fetch

It seems twitch is doing some sort of weird partial page update, if you try to open a video directly: https://www.twitch.tv/videos/1534401704 you'll see the twitter:description tag is not set to the channel bio, even if you navigate back to the about section or channel home.

I've found another api ViewerFeedback_Creator that should work everywhere, however it needs the channel ID and i haven't found a way to obtain it from the streams pages yet.

@riccardobl
Copy link
Contributor Author

riccardobl commented Jul 29, 2022

I've found a way to get the channel id, this should now work on all pages 2b6a015

}
)
)[0];
if (
Copy link
Contributor

@lujakob lujakob Jan 11, 2023

Choose a reason for hiding this comment

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

you could simplify this condition with Typescript optional chaining. See:

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining

if (channelData?.data?.video?.owner)

}
)
)[0];
if (channelData && channelData.data && channelData.data.userOrError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also could apply optional chaining here.

@bumi
Copy link
Collaborator

bumi commented Jan 15, 2023

yay, thanks for the PR! 🚀
@im-adithya you know the batteries well, can you do a review?

@im-adithya
Copy link
Member

Nice PR! Thanks @riccardobl 🚀

Just a few suggestions:

  • Please rebase (to remove conflicts) and use the common findLightningAddressInText function from the helpers file.
  • Optional chaining wherever possible (as @lujakob suggested above)
  • And please separate the handling of the videos and channels pages to increase readability (you can take the GitHub battery as an example)!

Also, is it possible to make an extractClientId function during the start of the battery and proceed only if we get one (and pass it on everywhere we make an API call)? I was thinking of avoiding the usage of browser storage. (it's not worth caching if we can find it by looping through the script tags anyways(?))

@im-adithya
Copy link
Member

@riccardobl are you still working on this?

@im-adithya im-adithya merged commit 842a24b into getAlby:master Apr 6, 2023
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.

5 participants