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

fix: render initial frame image from image handler #271

Closed
wants to merge 3 commits into from

Conversation

dalechyn
Copy link
Collaborator

Previously, frog:image meta tag was used as a workaround that held the
full image URL which has an image query parameter that contains a
compressed serialized image itself.
However, initial frame image doesn't have c.frameData-based constraints
as it is not present on the initial render.

This change moves the initial image render responsibility to the /image
handler itself, removing the request from an /image route one level up
to get the full URL from frog:image, and removes this meta tag as
well.

Double request or a request within self has created issues with wrangler
deployments (that I tried to fix in #268 by opting out from frog:image
tag and falling back to long initial frame image URL), and created
issues within Vercel Edge deployments that treat such a request as an
"Infinite" one.

Supersedes #268.

Previously, `frog:image` meta tag was used as a workaround that held the
full image URL which has an `image` query parameter that contains a
compressed serialized image itself.
However, initial frame image doesn't have `c.frameData`-based constraints
as it is not present on the initial render.

This change moves the initial image render responsibility to the `/image`
handler itself, removing the request from an `/image` route one level up
to get the full URL from `frog:image`, and removes this meta tag as
well.

Double request or a request within self has created issues with wrangler
deployments (that I tried to fix in wevm#268 by opting out from `frog:image`
tag and falling back to long initial frame image URL), and created
issues within Vercel Edge deployments that treat such a request as an
"Infinite" one.

Supersedes wevm#268.
Copy link

vercel bot commented Apr 19, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
frog ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2024 2:48pm
frog-frame ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2024 2:48pm

@dalechyn dalechyn self-assigned this Apr 19, 2024
Copy link

vercel bot commented Apr 19, 2024

@dalechyn is attempting to deploy a commit to the wevm Team on Vercel.

A member of the Team first needs to authorize it.

src/frog-base.tsx Outdated Show resolved Hide resolved
@dalechyn
Copy link
Collaborator Author

NextJS template with Edge environment enabled and with a patch from this commit: preview, repo.

Vercel template preview, repo.

@dalechyn dalechyn requested a review from jxom April 19, 2024 14:55
@dalechyn dalechyn marked this pull request as ready for review April 19, 2024 14:55
initialState: this._initialState,
origin,
})
const response = await handler(context)
Copy link
Member

Choose a reason for hiding this comment

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

If we go down this route, then this will cause a double render cycle on the initial request (one render cycle on the main endpoint, and one on this /image endpoint). This means it will become non-idempotent (ie. operations in the handler will be invoked twice which could cause db/store related bugs).

I wonder if there is a way to do this without invoking the handler again.

Copy link
Member

@jxom jxom Apr 19, 2024

Choose a reason for hiding this comment

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

I still need to play around with the initial image caching issue, but why would WC cache the initial image if cache-control: max-age=0 on the frame endpoint? Shouldn’t it always serve fresh frame data and seems like a WC issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The issue is that WC caches an image by url, and if an image changes its url also changes because the compressed image query parameter is changing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Therefore since every "refreshed" image has a unique link, wc will cache the initial one and would expect the initial one to be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we go down this route, then this will cause a double render cycle on the initial request (one render cycle on the main endpoint, and one on this /image endpoint). This means it will become non-idempotent (ie. operations in the handler will be invoked twice which could cause db/store related bugs).

I wonder if there is a way to do this without invoking the handler again.

You're absolutely right.
We need to think of a different solution then.

Copy link

@madroneropaulo madroneropaulo Apr 21, 2024

Choose a reason for hiding this comment

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

https://warpcast.com/horsefacts.eth/0x66aac3d2 IMO the cleaner way to handle this. It's a small step for the user but the code is even cleaner. So we could just revert and include instructions in the documentation of provide some sort of middleware to make it easier to use.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jxom, if idempotency is our main issue in this PR, we could enforce frame image handler to always be invoked after the frame handler.

Currently, frog implements those to be independent from each other - meaning that given a frame response you're given a specific frame image URL that always renders "independently" from a frame. As in fact if you serialize the query parameters correctly, you could render any image even outside of the frame context.

However, all the app clients are fetching frames first, and retrieve image urls as a result, thus the frame image in fact depends on the frame response itself.

I propose to implement an initial frame image storage inside of frog which would enforce triggering only one of the handlers and returning a stores value in another one.

Here's the first example, where a Frame is casted for the first time (meaning that frame is not cached in the app client):

  1. App client requests a frame. Frame handler is executed, where image is one of the returns. If the context.status === "initial", we put the image in a local storage where the key is some kind of unique requestId (can we get it from somewhere?), which stays the same within a single full frame fetch operation..
  2. App client requests for frame image. Image handler is executed. Since frame handler was executed before, we have the image in local storage by the given requestId, so we just return it.

And here's the second example where the app client has the frame cached.
The example is also only applicable for requests where context.status === "initial"

  1. Since the frame response is cached indefinitely, app client makes a fetch for frame image (given that the age of the cached frame image request has surpassed max-age).
  2. Local storage doesn't have a cached value for that specific requestId (because frame handler was not invoked), so the image handler invokes frame handler just to retrieve the image, and doesn't store anything in the local storage since there will be no requests afterwards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jxom, if idempotency is our main issue in this PR, we could enforce frame image handler to always be invoked after the frame handler.

Currently, frog implements those to be independent from each other - meaning that given a frame response you're given a specific frame image URL that always renders "independently" from a frame. As in fact if you serialize the query parameters correctly, you could render any image even outside of the frame context.

However, all the app clients are fetching frames first, and retrieve image urls as a result, thus the frame image in fact depends on the frame response itself.

I propose to implement an initial frame image storage inside of frog which would enforce triggering only one of the handlers and returning a stores value in another one.

Here's the first example, where a Frame is casted for the first time (meaning that frame is not cached in the app client):

  1. App client requests a frame. Frame handler is executed, where image is one of the returns. If the context.status === "initial", we put the image in a local storage where the key is some kind of unique requestId (can we get it from somewhere?), which stays the same within a single full frame fetch operation..
  2. App client requests for frame image. Image handler is executed. Since frame handler was executed before, we have the image in local storage by the given requestId, so we just return it.

And here's the second example where the app client has the frame cached. The example is also only applicable for requests where context.status === "initial"

  1. Since the frame response is cached indefinitely, app client makes a fetch for frame image (given that the age of the cached frame image request has surpassed max-age).
  2. Local storage doesn't have a cached value for that specific requestId (because frame handler was not invoked), so the image handler invokes frame handler just to retrieve the image, and doesn't store anything in the local storage since there will be no requests afterwards.

just figured out that all of that is applicable without this requestId, there's no need to preserve the "origin" of the request as the context is empty – GET requests have no payload in them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

such a "storage" most likely won't be reliable in serverless environments where a worker would kill itself in between frame and frame image requests causing idempotency breakage

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

such a "storage" most likely won't be reliable in serverless environments where a worker would kill itself in between frame and frame image requests causing idempotency breakage

however, the delay between such request is so small that it's less likely that a pod would go to sleep

@dalechyn
Copy link
Collaborator Author

putting this PR back to draft as there is an effort ongoing on changes for caching policies for frames.

@dalechyn dalechyn marked this pull request as draft April 24, 2024 17:02
@dalechyn
Copy link
Collaborator Author

closing this pr in favor of a spec change

@dalechyn dalechyn closed this Apr 26, 2024
@dalechyn dalechyn reopened this May 2, 2024
@dalechyn dalechyn mentioned this pull request May 2, 2024
@dalechyn
Copy link
Collaborator Author

dalechyn commented May 2, 2024

Superseded by #294.

@dalechyn dalechyn closed this May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
D: Medium Difficulty: Medium P: High Priority: High T: Bug Type: Bug T: Refactor Type: Refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants