Skip to content
This repository was archived by the owner on Feb 10, 2025. It is now read-only.

fix(cloudflare): allow compile-time image optimization for prerendered pages#53

Closed
stancl wants to merge 2 commits intowithastro:mainfrom
stancl:cf-image-service-fix
Closed

fix(cloudflare): allow compile-time image optimization for prerendered pages#53
stancl wants to merge 2 commits intowithastro:mainfrom
stancl:cf-image-service-fix

Conversation

@stancl
Copy link
Copy Markdown

@stancl stancl commented Nov 5, 2023

Changes

TLDR of the issue: the driver currently unnecessarily blocks image optimization even if it could be used just fine on static pages. The only issue is with including the image services in the SSR bundle.

I was more or less trying to reimplement the solution discussed on Discord but realized we don't actually need to re-export the services here, since the entire point of that was bypassing the if check in this code I believe.

So the only change now is adding an endpoint.

TODOs:

  • Implement the endpoint so that it points to an existing file
  • Not sure how this will work with SSR? We want to use the noop driver on SSR <Image> tags. Maybe the endpoint can handle that? Otherwise we could maybe specify the entrypoint conditionally, i.e. something like this (I think?)
    service: import.meta.env.DEV ? config.image.service : passthroughImageService()

@alexanderniebuhr

Testing

Docs

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Nov 5, 2023

⚠️ No Changeset found

Latest commit: 42fa3a3

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

@alexanderniebuhr
Copy link
Copy Markdown
Member

@stancl, my apologies for the delayed response; I was finalizing a component to share here. From our Discord conversation, we recognized that incorporating a solution for optimization at build/compile time, as well as for server-side rendering (SSR), adds a level of complexity that may exceed the scope of this current PR.

Nonetheless, we agreed that it's beneficial to provide users with the flexibility to choose between build/compile time optimization and runtime optimization. In line with this, and leveraging my work on an external image service, I've introduced an adapter configuration flag that can easily accommodate new options. I propose we extend this configuration to support build/compile time optimization as a selectable option.

https://github.com/withastro/adapters/pull/34/files#diff-be36604f10bbd9205e3930ca4cdb26388354a780fe7a8407735067cbff7694ecR164-R172

Regarding implementation, we have a few paths forward:

a) You could rebase your branch onto the feature branch and incorporate this addition.
b) Alternatively, you might prefer to wait until the feature branch is merged into the main branch and then introduce your changes.
c) As a third option, I can create a new branch to integrate your changes.

I would advocate for options a or b to ensure that you receive full credit for this contribution, though I am mindful of the extra effort this may entail. Please share your preference on how you wish to proceed.

@stancl
Copy link
Copy Markdown
Author

stancl commented Nov 8, 2023

I'm fine with you implementing this in your own branch, the PR was more so to provide something more direct (even if very incomplete) than opening an issue. The concept behind the solution is Erika's, not mine, so no need for any credit here :)

@alexanderniebuhr
Copy link
Copy Markdown
Member

Closing in favor of #57
I'll implement it in another branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants