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

feat: make the open graph tags dynamic based on the base64 and url query parameters #1122

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

helios2003
Copy link
Contributor

@helios2003 helios2003 commented Jul 9, 2024

This PR addresses the issue mentioned in #224. I have added the following features in this PR.

Features Added

  • Add a middleware that captures Open Graph crawler requests when they hit the website and rewrites the URL to the route api/crawler. This is done to serve minimal HTML when the crawler hits the page, reducing the chance of timeout.
  • Parse the URL and send it to the Image Generator service to add the dynamic image.
  • Show the default og:image if there is an invalid base64 string provided.
  • Fall back on the usual behavior added in #1106 if there isn't a base64 string.

Todos

  • Address unusual behavior observed like Slack showing the default image even when the document is valid.
    • For example: if we go to https://studio-studio-next.vercel.app and use MQTT template, then the default behavior is observed instead of dynamic tags.
    • Sometimes, Slack crawlers arent able to detect the open graph image.
  • Figuring out why sometimes the dynamic og:image does not appear on site like Whatsapp.
  • Add additonal crawlers to the list.

cc : @smoya

Copy link

changeset-bot bot commented Jul 9, 2024

⚠️ No Changeset found

Latest commit: 35c4a72

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Jul 9, 2024

Deploy Preview for modest-rosalind-098b67 ready!

Name Link
🔨 Latest commit 35c4a72
🔍 Latest deploy log https://app.netlify.com/sites/modest-rosalind-098b67/deploys/6734d5b982fb020008a9961f
😎 Deploy Preview https://deploy-preview-1122--modest-rosalind-098b67.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 9, 2024

Deploy Preview for asyncapi-studio-design-system ready!

Name Link
🔨 Latest commit 35c4a72
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-studio-design-system/deploys/6734d5b97770160008d6da62
😎 Deploy Preview https://deploy-preview-1122--asyncapi-studio-design-system.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jul 9, 2024

Deploy Preview for studio-next ready!

Name Link
🔨 Latest commit 19858da
🔍 Latest deploy log https://app.netlify.com/sites/studio-next/deploys/66e98efe0094a20008517e8b
😎 Deploy Preview https://deploy-preview-1122--studio-next.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

left some comments.

async rewrites() {
return [
{
source: '/:base64',
Copy link
Member

Choose a reason for hiding this comment

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

I think we can support URLs here as well, I mean when a user shares a file with url parameter.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should support both base64 and url use cases as stated in #224

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure, I will add it.

apps/studio-next/src/app/api/crawler/route.tsx Outdated Show resolved Hide resolved
import { Parser } from '@asyncapi/parser';
import { DocumentInfo } from '@/types';

export default async function parseURL(base64Document: string): Promise<DocumentInfo | null> {
Copy link
Member

Choose a reason for hiding this comment

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

We also have this parsing logic to parse the AsyncAPI file from base65 or a url in the studio as well, maybe we can re-use this functionality in both places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So do you recommend keeping the studio version, or using this code?

Copy link
Member

Choose a reason for hiding this comment

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

They both serve the same purpose, so I guess you can look at both of them and combine them into a util function.

Copy link
Member

Choose a reason for hiding this comment

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

The configuration and this middleware achieve the purpose, right? I don't understand why we are adding both.

@helios2003
Copy link
Contributor Author

Thank you for the review @KhudaDad414. I will add the suggested changes.

@helios2003
Copy link
Contributor Author

Also, I had a few questions:

  • Is it okay if I hardcode the popular social media site crawlers as I have done now or is it better if we use a library like https://github.com/monperrus/crawler-user-agents as suggested to me by @smoya. The main problem I could think of is that by including all the potential sets of crawlers as mentioned in the repository we could be causing rewrites for nonopen graph crawlers as well which is not required. But @smoya mentioned that he does not expect other bots to use the query parameters like that of open graph crawlers.
  • Also, could you please let me know your thoughts on https://github.com/helios2003/ogp-studio and its code?

@smoya
Copy link
Member

smoya commented Jul 17, 2024

  • Is it okay if I hardcode the popular social media site crawlers as I have done now or is it better if we use a library like https://github.com/monperrus/crawler-user-agents as suggested to me by @smoya. The main problem I could think of is that by including all the potential sets of crawlers as mentioned in the repository we could be causing rewrites for nonopen graph crawlers as well which is not required.

If you don't use a complete list of crawlers, you will end up facing issues like the one you mentioned:

Figuring out why sometimes the dynamic og:image does not appear on site like Whatsapp.

This is happening most probably because you are not considering Whatsapp User-Agent(s).

But @smoya mentioned that he does not expect other bots to use the query parameters like that of open graph crawlers.

AFAIK, indexer robots such as Google only follow links, meaning that unless there is a published link somewhere to a link such as https://studio.asyncapi.com/?base64=<base64_doc> or the variant with url query param, the parsing won't happen. In fact, it does make sense to do the parsing and return the proper title, description, etc to the results of such indexer bot request so it appears in the results (like google results).

@smoya
Copy link
Member

smoya commented Jul 17, 2024

Will that work in a Netlify setup? I mentioned an alternative in case it doesn't in #224.

Additionally, I think we can include more statistics in the rendered image like # of operations

@helios2003
Copy link
Contributor Author

Will that work in a Netlify setup? I mentioned an alternative in case it doesn't in #224.

Yes, it works. The service is currently hosted on Netlify: https://ogp-studio.netlify.app.

Additionally, I think we can include more statistics in the rendered image like # of operations

Okay, I will add it.

@helios2003 helios2003 changed the title feat: make the open graph tags dynamic based on the base64 url feat: make the open graph tags dynamic based on the base64 and url query parameters Jul 25, 2024
@helios2003
Copy link
Contributor Author

Added modifications:

@helios2003 helios2003 requested review from KhudaDad414 and smoya July 26, 2024 07:38
<meta property="og:title" content="AsyncAPI Studio" />
<meta property="og:description" content="Studio for AsyncAPI specification, where you can validate, view preview documentation, and generate templates from AsyncAPI document." />
<meta property="og:url" content="https://studio-next.netlify.app" />
<meta property="og:image" content="https://raw.githubusercontent.com/asyncapi/studio/master/apps/studio-next/public/img/meta-studio-og-image.jpeg" />
Copy link
Member

Choose a reason for hiding this comment

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

Can those values be grabbed from

url: 'https://studio-next.netlify.app',
?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, @smoya

Comment on lines 28 to 30
export const config = {
matcher: ['/:base64', '/:url'],
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this matcher configuration correct? I feel like it is matching to path parameters instead of query parameters that we use for url and base64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yes, you are right, I didn't interpret the documentation correctly, I will make the changes.

const websiteTitle = "AsyncAPI Studio";
const websiteDescription = "Studio for AsyncAPI specification, where you can validate, view preview documentation, and generate templates from AsyncAPI document.";
const websiteUrl = "https://studio-next.netlify.app";
const ogImage = "https://raw.githubusercontent.com/asyncapi/studio/master/apps/studio-next/public/img/meta-studio-og-image.jpeg";
Copy link
Member

Choose a reason for hiding this comment

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

As I commented previously, those values are already present in the source code in

import ogImage from '@/img/meta-studio-og-image.jpeg';
export const metadata: Metadata = {
metadataBase: new URL('https://studio-next.netlify.app'),
openGraph: {
type: 'website',
title: 'AsyncAPI Studio',
description: 'Studio for AsyncAPI specification, where you can validate, view preview documentation, and generate templates from AsyncAPI document.',
url: 'https://studio-next.netlify.app',
images: [
{
url: ogImage.src,
width: 800,
height: 600,
alt: 'AsyncAPI default image',
},
]
},
twitter: {
site: '@AsyncAPISpec',
}
}

Please reuse them somehow instead of hardcoding them here again.

@helios2003
Copy link
Contributor Author

The PR is ready for review @KhudaDad414, @smoya.

@helios2003
Copy link
Contributor Author

Also, I'm unsure if the middleware code I wrote is the best way to achieve this functionality. Any suggestions or improvements would be appreciated.

</head>
</html>
`;
console.log(crawlerInfo);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
console.log(crawlerInfo);

@smoya
Copy link
Member

smoya commented Aug 25, 2024

@helios2003 Are you planning to move the OG Image generator service (Netlify Edge Function) into this PR?

@helios2003
Copy link
Contributor Author

Maintainers, can you please review the PR? I have addressed all the comments above and included the unit tests. Additionally, I had to change the version of @asyncapi/converter after this commit: asyncapi/converter-js@b3592ef.
I did not include these changes in the studio repository since we are soon switching to studio-next as the default version for studio based on this issue: #1134

@Amzani
Copy link
Collaborator

Amzani commented Sep 13, 2024

Some tests are still failing @helios2003

@helios2003
Copy link
Contributor Author

@Amzani the tests are failing in studio which I did not update as mentioned above.

@Shurtu-gal
Copy link
Collaborator

I suppose this can me merged as soon as the merging of studio and studio-next is done.

Copy link
Member

@KhudaDad414 KhudaDad414 left a comment

Choose a reason for hiding this comment

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

tests are faliling since you are installing a newer version of jest-environment-jsdom on studio-next app explicitly.

to resolve install the older version in studio app.
pnpm install -D [email protected]

Copy link

@helios2003
Copy link
Contributor Author

Maintainers, the tests are passing now. PTAL at the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
Development

Successfully merging this pull request may close these issues.

Response time for NextJS version seems too high
5 participants