-
-
Notifications
You must be signed in to change notification settings - Fork 40
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 Mastodon support #140
base: main
Are you sure you want to change the base?
Add Mastodon support #140
Conversation
We want to ignore `demo/.astro/settings.json` containing the last update check date
|
✅ Deploy Preview for astro-embed ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @HiDeoo! Still didn’t have time for a full review, so thought I’d at least post my notes from last week before I forget about them.
// Thanks to eleventy-plugin-embed-mastodon | ||
// https://github.com/inframanufaktur/eleventy-plugin-embed-mastodon/blob/f8c1687abf1f88a6351e53e38f91aff0c95a5cb2/.eleventy.js#L46-L47 | ||
const urlPattern = | ||
/(?:https:\/\/)?([\w\d-]*?.?[\w\d-]*.[a-z]*\/@[\w\d_]*(?:@[\w\d]*?.?[\w\d]*.[a-z]*)?\/)(\d*)/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering about how this would work given there are theoretically unlimited Mastodon domain names.
Feels really tricky. For example, https://www.youtube.com/@astrodotbuild/
matches this regular expression, but is not a toot. And I guess there are likely other sites adopting this @username
style of URL.
Do you know if there’s a reason for the \d
portion at the end here to be optional? I’d guess \d+
instead of \d*
would at least mean you get slightly less false positives.
(None of this is relevant to the component version, but it is in the integration usage where in theory this pattern might “steal” a match from something else.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call. I updated the regex to enforce the last digit altho like you said, it's a bit tricky in this case. May need to think about it more but the only ideas that are coming to mind right now are not having Toot
enabled as a service
by default with an explanation in the docs or just not supporting Mastodon in the integration. Both of which are not ideal but the fact that the domain can be anything is a bit complex to handle I guess in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a strong opinion as we don’t really have a standard around this, but wondering if <Toot>
is standard enough to be used as the component name vs something more verbose like <MastodonPost>
(officially Mastodon always calls them “posts”, but maybe the colloquial “toot” is OK here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have the expertise to make a decision I think considering I don't use the service. I mostly only mimicked the Twitter example so far.
While digging a bit more, I found this article and PR which also suggests that "post" might be a better term to use instead of "toot".
Happy to refactor if you think it's a good idea.
const tootId = extractID(id); | ||
if (!tootId) throw new Error('Invalid toot URL'); | ||
const apiUrl = new URL(`${new URL(id).origin}/api/v1/statuses/${tootId}`); | ||
const response = await fetch(apiUrl); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other components use a “safe” fetching method from the utils package:
astro-embed/packages/astro-embed-utils/index.ts
Lines 28 to 33 in 6fce869
/** | |
* Fetch a URL and parse it as JSON, but catch errors to stop builds erroring. | |
* @param url URL to fetch | |
* @returns {Promise<Record<string, any> | undefined>} | |
*/ | |
export const safeGet = makeSafeGetter<Record<string, any>>((res) => res.json()); |
This helps avoid breaking builds and also has a built-in caching layer to speed up repeat fetches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I totally missed that as I mostly based my work on the Tweet component which does not use it (I guess this would be another PR). Updated the PR to use it. Thanks for pointing it out!
In less than 1 week, the user I found with an emoji in their username (and these are difficult to find) already removed it making the test fail.
Description
This PR adds support for embedding posts from Mastodon.
The content is fetched using the Mastodon API as discussed due to the fact that the Mastodon oEmbed API only returns an
iframe
and ascript
tag.It currently supports the following feature:
image
,video
,gifv
andaudio
media attachmentsThe PR is a draft as it still needs some work to be done and some questions to be answered:
iframe
with themastodon-embed
class