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

Improve meta handling (NOT included => default Farcaster Frame) #811

Merged
merged 8 commits into from
May 7, 2024

Conversation

carletex
Copy link
Member

@carletex carletex commented Apr 12, 2024

I was thinking about adding a default "dummy" Farcaster Frame for SE-2. I say "dummy" because you usually add a backend route to handle different actions within the frame (but that would be too much)

To test locally, you can use: https://github.com/coinbase/onchainkit
-> clone, go to the framegear folder, npm install, npm run dev... and you get this:

frames


A few things:

  • Is this worth adding?
  • About NextJS Metadata. layout.tsx and getMetadata.ts feel a bit duplicated. Is there a way around it? Feels weird to have to add the frame meta to both places.
  • VERCEL_URL: We use it for the base URL, and now Vercel returns the "protected deployment route" (instead of the production one. Related to Vercel CLI gives gated link instead of public URL #807 I think).. which make this a bit useless:
const baseUrl = process.env.VERCEL_URL
  ? `https://${process.env.VERCEL_URL}`
  : `http://localhost:${process.env.PORT || 3000}`;

Any ideas? (we can discuss in another issue/PR, sorry haha)

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

Nicceee, Thanks @carletex !! Looks great and I think its nice to have!


Things which we can handle in maybe different PR :

About NextJS Metadata. layout.tsx and getMetadata.ts feel a bit duplicated. Is there a way around it? Feels weird to have to add the frame meta to both places.

Yup agree, also with vercel giving gated URL its PITA to that baseUrl in both files

I like the way how vercel in their docs have created shared-metadata.ts file, maybe we could have that for all the common things in metadata ?

Docs Image :

Screenshot 2024-04-15 at 8 45 35 PM

VERCEL_URL: We use it for the base URL, and now Vercel returns the "protected deployment route" (instead of the production one. Related to #807 I think).. which make this a bit useless:

:( , Just created vercel/vercel#11438 and also asked question on discord

@carletex
Copy link
Member Author

(Sorry for deviating from the original PR haha)

About NextJS Metadata. layout.tsx and getMetadata.ts feel a bit duplicated. Is there a way around it? Feels weird to have to add the frame meta to both places.

Seems like the obvious way to fix it. What am I missing here?

cb47ae9

Besides that, we could even add baseUrl to scaffold.config.ts.

@technophile-04
Copy link
Collaborator

technophile-04 commented Apr 16, 2024

Seems like the obvious way to fix it. What am I missing here?

cb47ae9

Niiiceee !!!

Screenshot 2024-04-16 at 6 41 04 PM

I thought having templates inside getMedata would render home page metadata title as "Scaffold-ETH 2 App | Scaffold-ETH 2" but it doesn't and works nicely because of this and it also nicely renders other pages with template !

we could even add baseUrl to scaffold.config.ts

+1 for this, looking at Next's discord people have been using / suggesting to hardcode the domain URL directly and there's no way to get domain from vercel's env var

So makes sense to have it in scaffold.config.ts

Nitpick: Maybe we could use domainUrl / domain (staright forward interpretation) instead of baseUrl (lol this seems a broader term and might not be that clear) <= this could be solve with a one liner comment above it though 🙌

Tysm @carletex !!

@carletex carletex changed the title Add metas for a default Farcaster Frame Improve meta handling + default Farcaster Frame Apr 16, 2024
@rin-st
Copy link
Collaborator

rin-st commented Apr 16, 2024

So makes sense to have it in scaffold.config.ts

+, tried to find a better way to set baseUrl too, but no luck


Thanks for pr and interesting discussion!

@carletex
Copy link
Member Author

Thanks all for the comments!!

I'm gonna do some tinkering and propose an initial solution.

Also trying this now: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase

@carletex
Copy link
Member Author

carletex commented May 1, 2024

The metadataBase doesn't seem to work for me: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase

image

This would be nice too.... but we have the VERCEL_URL issue, where it points to a usually protected deployment route (instead of the PROD URL)

If we don't make it work, maybe we just can leave it as it's in the PR (at least we are deduplicating the layout / getMetadata code)

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

The metadataBase doesn't seem to work for me: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase

:( , But I really love now have we have centralised / one getMetadata function ! Really easy to edit !!

Thanks Carlos !!!

@carletex carletex changed the title Improve meta handling + default Farcaster Frame Improve meta handling (NOT included => default Farcaster Frame) May 7, 2024
@carletex
Copy link
Member Author

carletex commented May 7, 2024

So for now, decided to remove the Farcaster frame stuff from this PR. See this convo for context: #811 (comment)

So at the end this PR just remove some duplication with the metadata.

plus

https://twitter.com/rauchg/status/1787664435097538776

This happened finally!! 🔥 🔥

Copy link
Collaborator

@Pabl0cks Pabl0cks left a comment

Choose a reason for hiding this comment

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

Love how metadata is handled now, and the new VERCEL_PROJECT_PRODUCTION_URL!

Looking good to me 🔥 GJ @carletex

=============================

I think this behavior is OK, since vercel only saves 1 variable for the project, but just checking:

Testing the unfurl with the production link, realized only works if you share the main domain from the project, not the deployment URL.

image
image

Copy link
Collaborator

@technophile-04 technophile-04 left a comment

Choose a reason for hiding this comment

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

The metadataBase doesn't seem to work for me: https://nextjs.org/docs/app/api-reference/functions/generate-metadata#metadatabase

image

This would be nice too.... but we have the VERCEL_URL issue, where it points to a usually protected deployment route (instead of the PROD URL)

If we don't make it work, maybe we just can leave it as it's in the PR (at least we are deduplicating the layout / getMetadata code)

Maybe this will solved in latest Next version not completely sure though ? https://x.com/WillemJaap_/status/1787738993964048560

realized only works if you share the main domain from the project, not the deployment URL.

Yes, actually this problem is kind of related to #807....the deployment URL are unique and only accessible to team / authenticated users and hence when you share that URL in whatsapp / any other app, that app won't be able access / fetch anything from that URL

If you try to open https://new-metadata-47koiot3h-tokodevs-projects.vercel.app in incognito it won't work there too.

Merging this thanks all !!

@technophile-04 technophile-04 merged commit 3ff50f2 into main May 7, 2024
1 check passed
@technophile-04 technophile-04 deleted the frames branch May 7, 2024 12:44
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.

None yet

5 participants