-
Notifications
You must be signed in to change notification settings - Fork 440
Add openimg for image optimization #935
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
Add openimg for image optimization #935
Conversation
.gitignore
Outdated
@@ -1,6 +1,7 @@ | |||
node_modules | |||
.DS_store | |||
|
|||
/data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only needed if the default filesystem caching is enabled. Default caching destination should work out of the box with the current Fly setup!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand it right, you're putting the caching destination as the current working directory/data
. And I worry that that is going to get blown away on every redeploy of the app. I think that we might want to configure that to go somewhere that would be persisted between deploys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points. The caching strategy depends on how the Epic Stack is deployed. By default, optimized images are cached to ./data/images
but this can be configured via the cacheFolder
argument: string
| no_cache
(API reference). Since the Epic Stack is hosted on Fly with a mounted volume, I assumed the /data
folder would persist between deployments:
[mounts]
source = "data"
destination = "/data"
I guess every machine has its own volume/image cache in the current setup, but I believe they persist?
I guess our options are:
- Quasi-persistent cache to the filesystem at
/data
if Fly persists the volumes per machine in the current setup - If we have a CDN like Cloudflare running in front of the machines, then HTTP caching is probably sufficient with
cacheFolder: "no_cache"
. - Synchronized caching across all machines via SQLite as a cache location. This would require some code changes to openimg, but happy to add that via a
customCache
config option or similar!
Lmk what you would prefer!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're sticking things in ./data/images
, that's going to be relative to the repo, not the mounting destination (right now, I think this is going into /data/myapp/data/images
. So you'll want to make sure that it's not relative to the repo, but rather the specific mount destination, which will make things persisted.
We do not have a CDN like Cloudflare running in front of the machines. Our server serves up all the static assets directly.
I think the most correct option here would be to use Tigris for this. I was planning on doing Tigris soon anyway, so I'll get that set up, and then we can get this working with Tigris as our storage location. Do you think that would work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are so right. 🤦 Well, thanks for catching this! Fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love this a lot, just a couple of things.
Could you add:
Thanks! |
Hey Kent, thanks for looking into this PR so quickly and for the great discussions! I added a first draft for the decision doc and documentation for image optimization (including the existing image logic). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for giving me the motivation I needed to get Tigris working for the Epic Stack 😅
.gitignore
Outdated
@@ -1,6 +1,7 @@ | |||
node_modules | |||
.DS_store | |||
|
|||
/data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're sticking things in ./data/images
, that's going to be relative to the repo, not the mounting destination (right now, I think this is going into /data/myapp/data/images
. So you'll want to make sure that it's not relative to the repo, but rather the specific mount destination, which will make things persisted.
We do not have a CDN like Cloudflare running in front of the machines. Our server serves up all the static assets directly.
I think the most correct option here would be to use Tigris for this. I was planning on doing Tigris soon anyway, so I'll get that set up, and then we can get this working with Tigris as our storage location. Do you think that would work?
Thank you so much for working on this. It really got me going on Tigress hosting, and I'm excited to integrate this with what I have coming very soon. I'm sorry, but you're going to have a couple of merge conflicts. I hope it's not very significant, but when we're all done, this is going to be so epic. If you're curious, here's the live stream: https://www.youtube.com/watch?v=flZCS8KthzU I got so close to being done! You can also check the dev branch which has all the work. Pretty sure it's ready. |
I have successfully finished the migration to Tigris for storing our images. If you could update your PR to handle any changes, that would be great. Hopefully, it's pretty minimal. This will be much better. Thank you! |
app/routes/resources+/images.tsx
Outdated
headers.set('Cache-Control', 'public, max-age=31536000, immutable') | ||
return getImgResponse(request, { | ||
headers, | ||
allowlistedOrigins: [domain, process.env.AWS_ENDPOINT_URL_S3], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't actually fetch an image from Tigris directly just yet but I already added the process.env.AWS_ENDPOINT_URL_S3
in the allowlist.
Awesome! I didn't need to make any code changes because |
I think I would like the user and note images to use openimg as well. What do you think? |
They are! 100%, we want to build a solution that covers all images. The way note & user images are currently optimized is via this flow:
This is the configuration in if (params.src.startsWith('/resources')) {
// Fetch image from resource endpoint
return {
type: 'fetch',
url: domain + params.src,
}
} The indirection I wanted to highlight is making a fetch request to another endpoint on the same server. It's fine as it only happens on cache miss (first request), but we could still optimize it. For instance, we could remove the resource routes and just include the signed URLs to the remote images in the user and note objects in the loader functions. Then the |
Co-authored-by: Kent C. Dodds <[email protected]>
Co-authored-by: Kent C. Dodds <[email protected]>
Co-authored-by: Kent C. Dodds <[email protected]>
Co-authored-by: Kent C. Dodds <[email protected]>
Co-authored-by: Kent C. Dodds <[email protected]>
Co-authored-by: Kent C. Dodds <[email protected]>
1f3ba11
to
2dc20d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I never like it when a server makes requests to itself, so I've updated this PR to not do that, but we need some changes in openimg for this to work. Could you look into that?
app/routes/resources+/images.tsx
Outdated
const { url, headers } = getSignedGetRequestInfo(userImage.objectKey) | ||
return { | ||
type: 'fetch', | ||
url, | ||
headers, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need the fetch request you make to include these headers.
I also need getImgSource
to support returning a promise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Available via openimg
v0.2.0! 🎉
We're very close! This is a big improvement! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the changes you made. It's much more straightforward now. Added one comment only!
Hey @kentcdodds, I went through a few iterations, but pretty happy now with the outcome. Lmk what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, this is way cool!
Hey! Currently, the Epic Stack doesn't have a solution for image optimization and I would like to bring this up to discussion. I have been working on openimg, a small image optimization toolkit, as I have always wanted a small simple solution particularly for my Remix/React Router apps. I think it's simple enough to be included and provide value right out of the gates!
openimg
The goal of openimg is to be easy to use (inspired by Vercel's image optimization services) but also highly scalable and configurable, so you can reconfigure it (or replace it) as your app grows.
Features:
?react
You can start simple by adding a new resource route for the image optimization and then query images from the endpoint (see this PR). Next, you can add the
Img
React component to upgrade each image on your site incrementally (also already included in this PR). If you grow to have a lot of images, you can easily break out the optimization endpoint into a standalone server ("for scale") and only change a few lines of code to redirect all optimization requests to the remote server. Further, you can both optimize locally hosted image and remote images (e.g., from an S3 bucket) and use file system caching (default) or just utilize a CDN with HTTP caching headers, this should make openimg configurable enough to work across different environments (serverless, long-running server).I have been using openimg in production on allthingsweb.dev where the images are stored on S3 and optimized and cached on a Fly-hosted React Router app. Very happy with how it's been working!
Epic Stack
I understand the Epic Stack attempts to reduce services, including the self-managed variety, as per the image decision doc. Still, I think image optimization is important enough to build a solid starting point that scales with your app for a truly epic experience!
The main goal of image optimization is to reduce the total bytes a user downloads. Smaller images can make a big difference, even on sites with only a few images. For example, on Kody's note details page, we can shrink the image load from 1112kb to 46.5kb by resizing and converting images to AVIF on the fly.
Before & After
Before, 456kb + 348kb + 308kb:
After, 19kb + 13.1kb + 24.4kb:
Note that the initial resizing work does increase the response times for the first load (~from 10ms locally to ~200-300ms):
However, this is a trade-off for the first request of a given image (w, h, format) only. After the initial resizing and transformation, we can cache the resized image on the filesystem (default) and/or via CDN & HTTP caching. Using file system caching, we can then even slightly reduce the load times because we need to load less bytes (see second screenshot).
HTML picture element
The
Img
component fromopenimg/react
uses the picture element to query for the smallest possible image with modern formats, but can also fallback to the img element based on browser capabilities. The goal is to provide the least amount of information (just width & height) to create a good-enough image optimization approach. I personally don't want to deal withsrcset
andsizes
queries for every image that I add to my site.... but of course you can pass in and override all attributes.The default looks something like this:
Note you can add a
isAboveFold
prop to update theloading
,decoding
andfetchpriority
attributes to prioritize an image!With the changes in this PR, I was able to address the "use modern formats" lighthouse recommendation:
I believe these changes will help users get more mileage out of Epic Stack, but I would love to hear your thoughts on this topic!
Test Plan
TBD
Checklist
TBD
Screenshots
No visual changes, just smaller images in modern formats!