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: image handler #294

Merged
merged 17 commits into from
May 23, 2024
Merged

feat: image handler #294

merged 17 commits into from
May 23, 2024

Conversation

dalechyn
Copy link
Collaborator

@dalechyn dalechyn commented May 2, 2024

This PR is a result of long discussion with @deodad and @horsefacts. Really appreciate the inputs and effort put into helping with this change.

Problem

The current caching policies laid down in Frame Specs define the following:

  • Frame responses are indefinitely cached
  • Frame Images can pass Cache-Control header with a certain max-age=0 to revalidate frame image.

Frog generates image URLs on fly as an outcome of the frame handler response. Consider the following example:
/frame – Frame handler, and /frame/image – Frame image handler. Frame handler is always executed first and concatenates compressed image as a query parameter later to be added to /frame/image route in fc:frame:image meta tag response.
However, since frame responses are indefinitely cached, if a user passes Cache-Control: max-age=0 in imageOptions.headers, frame handler would still not be executed although the image URL would change as the consequence of the compressed image being changed.

In #222 and #271 I have tried to implement the support of such by introducing static frame image URLs, where the first frame (that is retrieved via GET http method and later cached indefinitely) have static image URLs (i.e. /frame/image and not /frame/image?image=aSDoafhetkahlsd...).

The problem with #222 was that frog:image tag was introduced that involved the image handler to fetch it in order to retrieve non-stripped image URL, did the fetch and returned it. It resulted in issues with Serverless Worker supporting environments such as Cloudflare Worker and Vercel Edge to not being able to resolve such as such workers cannot fetch themselves.

The problem with #271 was that idempotency was broken for the first frame request as it introduced a Frame Image handler that would work independently of the Frame handler. Meaning that if no compressed image was found in query parameters, an Image handler would assume it's the first frame request and would execute the handler once again.
While being an improvement over #222, it still had a bad code smell as user could have written custom logic on first frame image (i.e. to count how many times an image was refreshed?), which would have executed twice on the first request (i.e. that counter would always be +1 from the real result as the first uncached frame request would have incremented it).

Solution

As was suggested by @jxom, the solution is to add a separate image handler that can be used to generate an image in a specific and dedicated endpoint.

Bottlenecks

  • If used in not initial frames, by default satori (or hono-og) will add cache-control: max-age=366535 (some big value here), that is gonna let browsers cache the response, thus cache-control: max-age=0 must be added manually in imageOptions.
  • Image handler might potentially conflict with the internal image handler that is spawned by default under /image endpoint. i.e. if I define my own separate image handler that also ends with /image, the behavior is unexpected as from what it seems there is a race condition. Would be cool to have this statically checked.
Prev. iteration

If we truly consider the fact that the first frame request is indefinitely cached, we can in other words expect that the first frame response is static – meaning that the intents, aspect ratio, post url, frame image and everything else won't ever change unless frame url changes (or the embed data is scraped again forcefully).

Thus, the initial frame request is not needed to be handled in frame handler as there is no data on which one can depend on.

In that matter, I've added initial property to frame handler route options that is intended to be used as follows:

.frame(
  '/',
  (c) => {
    return c.res({
      image: (
        <VStack grow gap="4">
          <Heading color="text400">
            (POST) Current time: {new Date().toISOString()}
          </Heading>
        </VStack>
      ),
    })
  },
  {
    initial: {
      refreshing: true,
      image: () => (
        <VStack grow gap="4">
          <Heading color="text400">
            (GET) Current time: {new Date().toISOString()}
          </Heading>
        </VStack>
      ),
      intents: [<Button>POST request</Button>],
    },
  },
)

initial property in frame handler route options has all the same properties of FrameResponse which is normally returned in the frame handler, except for image property that might be a callback if refreshing: true is passed.

refreshing: true is passed whenever you want the first frame at that endpoint to "refresh". If it is marked as true, the frame handler would return this static response, call the image callback to retrieve the image and add Cache-Control: max-age=0 header to have the frame to become refreshable on each visit. It can also accept a seconds number which is put in that header's max-age=<number> value to have it refreshable with a distinct time period.

Passing refreshing: false on the other hand will force image property not to be a callback, but to have JSX.Element | string value instead, as if it never refreshes – it should be considered to be static.

Consequently, the frame handler itself would always receive POST http requests with frameData in them and would never be triggered with GET http requests.

Known limitations

  • One can pass refreshing: false if he wants his frame handler implementation to never be triggered in initial frame request, but that wouldn't make him able to make the old frames which are live currently to become refreshing and the new ones, unless he goes to the Developer / Embeds page, inserts the frame URL to drop the Frame Response cache on the following frames one would share.

Ways to come around those limitations

  • Instructing devs who build on frog in the docs & jsdocs to be aware of this and to manually scrape frame data again in case if they change such;
  • Instructing devs to copy over their frames if they want to implement "refreshing images" onto a another route to keep the previous frames alive.

@dalechyn dalechyn self-assigned this May 2, 2024
Copy link

vercel bot commented May 2, 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 May 23, 2024 11:51am
frog-auth ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 23, 2024 11:51am
frog-frame ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 23, 2024 11:51am

@dalechyn
Copy link
Collaborator Author

dalechyn commented May 4, 2024

What I need help with atm is figuring out wether Cache-Control or cache-control has to be set (or both) in the headers as different frameworks seem to treat such differently.

In the playground, the first letters need to be uppercased or otherwise it wouldn't be set.

In Vercel environments, those have to be lowercased.

Not aware of other environments atm.

@dalechyn
Copy link
Collaborator Author

dalechyn commented May 4, 2024

tested on all local environments with ngrok except for nextjs (has some invalid url issue), works as expected.

@dalechyn
Copy link
Collaborator Author

dalechyn commented May 4, 2024

wrangler: https://frog-template.frog-tests-dalechyn-static-image.workers.dev (works as expected).
vercel: frog-static-vercel-1.vercel.app/api (works as expected)
nextjs: frog-static-next-1.vercel.app/api (works as expected)

had to pass both cache-control and Cache-Control as wrangler and vercel parse such differently that results in two equal max-age=0,max-age=0 being present in the header, which imo isn't critical at all.

Copy link
Member

@tmm tmm left a comment

Choose a reason for hiding this comment

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

From a quick look, what do you think about changing the API design a bit. Introducing c.static instead of adding an initial property options.

export const app = new Frog({
  ui: { vars },
}).frame('/', (c) => {
  // handle both paths via same callback
  if (c.status === 'initial')
    return c.static({ // new `c.static`
      refreshing: true,
      image: () => (
        <VStack grow gap="4">
          <Heading color="text400">
            Current time: {new Date().toISOString()}
          </Heading>
        </VStack>
      ),
      intents: [<Button>Check again</Button>],
    })

  return c.res({
    image: (
      <VStack grow gap="4">
        <Heading color="text400">
          (clicked) Current time: {new Date().toISOString()}
        </Heading>
      </VStack>
    ),
  })
})

@dalechyn dalechyn changed the title feat: static initial frame image feat: static frame images May 21, 2024
@dalechyn dalechyn changed the title feat: static frame images feat: separate image handler May 21, 2024
@dalechyn dalechyn changed the title feat: separate image handler feat: image handler May 21, 2024
@dalechyn
Copy link
Collaborator Author

dalechyn commented May 21, 2024

@jxom, @tmm, added potential bottlenecks that are good to think about before merging this pr

Bottlenecks

  • If used in not initial frames, by default satori (or hono-og) will add cache-control: max-age=366535 (some big value here), that is gonna let browsers cache the response, thus cache-control: max-age=0 must be added manually in imageOptions.
  • Image handler might potentially conflict with the internal image handler that is spawned by default under /image endpoint. i.e. if I define my own separate image handler that also ends with /image, the behavior is unexpected as from what it seems there is a race condition. Would be cool to have this statically checked.

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

3 participants