Skip to content

Use Twitter OG tags if no Twitter credentials are configured - #522

Merged
pajlada merged 6 commits into
Chatterino:masterfrom
M4tthewDE:feature/twitter-opengraph-default
Sep 9, 2023
Merged

pajlada merged 6 commits into
Chatterino:masterfrom
M4tthewDE:feature/twitter-opengraph-default

Conversation

@M4tthewDE

@M4tthewDE M4tthewDE commented Sep 8, 2023 •

Copy link
Copy Markdown
Contributor

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Appends "(bot)" to User-Agent so that the default resolver can fetch OpenGraph tags for twitter links.
I don't like how this identifies twitter links, but it's the best I could come up with on the spot.

Note that the default resolver is only used if no Twitter token is specified in the config.

Closes #499

@codecov

codecov Bot commented Sep 8, 2023 •

Copy link
Copy Markdown

Codecov Report

Merging #522 (d18bc5b) into master (6cf160d) will increase coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #522      +/-   ##
==========================================
+ Coverage   46.90%   46.91%   +0.01%     
==========================================
  Files          99       99              
  Lines        3710     3717       +7     
==========================================
+ Hits         1740     1744       +4     
- Misses       1920     1922       +2     
- Partials       50       51       +1     
Files Changed Coverage Δ
internal/resolvers/default/link_loader.go 49.05% <50.00%> (-0.45%) ⬇️
internal/resolvers/twitter/resolver.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@M4tthewDE
M4tthewDE marked this pull request as ready for review September 8, 2023 13:18
@M4tthewDE M4tthewDE changed the title Fetch twitter opengraph tags Use twitter opengraph tags by default Sep 8, 2023
@KararTY

KararTY commented Sep 8, 2023 •

Copy link
Copy Markdown
Contributor

Had to look for a bit since I remember paj saying something:
image

the user-agent must only be added to the twitter requests

You're gonna have to ask pajlada if that's still the case, though.

@KararTY

KararTY commented Sep 8, 2023

Copy link
Copy Markdown
Contributor

Also if I'm not mistaken, #510 is also trying to solve the same underlying issue - The fact Twitter embeds don't work.

I don't know if there has been a discussion whether we'd go that route or this route with OG tags.

@M4tthewDE

Copy link
Copy Markdown
Contributor Author

I would consider falling back to OG tags by default to be desirable despite the other PR, since the API used there is unofficial.

I really don't like how this changes twitter/resolver.go,
but I haven't managed to figure out a better way to check if a request
matches.

@pajlada pajlada left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@pajlada pajlada changed the title Use twitter opengraph tags by default Use Twitter OG tags if no Twitter credentials are configured Sep 9, 2023
@pajlada
pajlada enabled auto-merge (squash) September 9, 2023 09:53
@pajlada
pajlada disabled auto-merge September 9, 2023 09:53
@pajlada
pajlada merged commit 1cab562 into Chatterino:master Sep 9, 2023
@M4tthewDE
M4tthewDE deleted the feature/twitter-opengraph-default branch September 9, 2023 19:06
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.

Twitter: Use OG resolver by default

3 participants