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: enable vercel image optimization #8667

Merged
merged 6 commits into from
Dec 12, 2023

Conversation

lettucebowler
Copy link
Contributor

@lettucebowler lettucebowler commented Jan 23, 2023

First PR, please excuse any mistakes I have made but I have done my best to follow the contribution guidelines

attempt to close #8561.

Exposes the necessary config to enable Vercel image optimization. Currently, with no accompanying component it won't be as convenient as something like Next/image, but is still much better than having no option at all. I'm currently running with these changes using pnpm patch on one of my current projects and I find it incredibly useful.

I wrapped the images config object in a platform object. My thought process was that it's not really a config option for the adapter itself, but rather a config option for the platform the build output will be deployed to. It also leaves room to enable future vercel features without adding a bunch of new root-level config-options to the adapter, they can just become part of the platform object.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

@changeset-bot
Copy link

changeset-bot bot commented Jan 23, 2023

🦋 Changeset detected

Latest commit: 885ba69

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-vercel Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann benmccann changed the title Feat: enable vercel image optimization feat: enable vercel image optimization Jan 23, 2023
@benmccann
Copy link
Member

If the other maintainers agree to add this we would need to document it in https://github.com/sveltejs/kit/blob/master/documentation/docs/25-build-and-deploy/90-adapter-vercel.md

@Rich-Harris
Copy link
Member

Thanks for the PR! The platform thing might be a bit weird, since there's already an event.platform object that relates to adapters. I don't know if that means it should be a top-level images property, or if we just need to rename it.

Can you describe how you'd use this in practice? If I understand correctly this would allow you do have images like this...

<img alt="..." src="/_vercel/image?url=/images/potato.jpg">

...but that wouldn't work in development. It feels like we might need a more integrated solution (which is part of a larger conversation around adapters being able to augment the dev server environment).

@hartwm
Copy link

hartwm commented Jan 23, 2023

<img alt="..." src="/_vercel/image?url=/images/potato.jpg">

Correct this is the usage. The pull request is only giving the ability to set the options in the build docs. Other than that it does nothing unless further functionality is implemented by the user. That said, I don't think that it needs to wait for a full solution in order to implement. I'd argue to allow it so that other people can build an image component, or custom pre-render function would solve for it in production.

@Rich-Harris
Copy link
Member

I definitely understand that sentiment, but there's a fine line between shipping incremental improvements in the spirit of 'don't let the perfect be the enemy of the good' and narrowing the eventual solution space. There's always a cost to adding new API, so it's important not to add something that you plan to make obsolete.

By adding this feature we're essentially saying 'this is how images should be optimised', but a) it creates a form of vendor lock-in (even if I work at Vercel, I don't want to make it harder for people to move between platforms), and b) there's a usage cost attached to runtime image optimisation, whereas optimising images at build time (with https://www.npmjs.com/package/vite-imagetools, for example) is free.

So for me, until we're able to offer an <Image> component that covers all the bases, the question is 'does this expose new capabilities that people need?' And I think the answer is 'no' — it's straightforward, if slightly more work, to edit .vercel/output/config.json after the adapter has created it.

@lettucebowler
Copy link
Contributor Author

lettucebowler commented Jan 23, 2023

I make it work in dev by using an image host environment variable. In dev the page serves the normal images from their normal paths, and in production it uses the _vercel/images?url= as the base path of the image. I don't think this really narrows future solution space, it just allows an alternative solution to image optimization.

Vite-imagetools is great for static assets at build-time, but it doesn't allow for optimization of user-submitted images because they won't exist to be imported and optimized. In that case your option is already basically go pay for cloudinary anyways, so I don't think the usage cost is a big deal. I think it's fine for a platform specific adapter to enable platform-specific features, and that's already being done with KV and durable objects on the cloudflare adapter, but I do think that the pricing and limitations are worth calling out in any documentation referencing the platform-specific features.

@hartwm
Copy link

hartwm commented Jan 23, 2023

The main reason for wanting this feature is because I find it very rare for the image source files to live in the project. They often have a CMS component and are served remotely. At the end of the day we are going to be paying someone to serve nextgen images and not statically creating all of them. In this case keeping it closer to the code source by it being a vercel product is an advantage for me in simplicity and accounting. The pricing seems fair.

If this option is not added to adapter package then you aren't even giving the company who is supporting the project the ability to gain more business for fear of nepotism and/or we will have to maintain our own forked adapters.

TLDR: Images coming from CMS can't take advantage of vitetools in a non-static environment and vercel has a viable option for quickly offloading that task and keeping image implementation within the project scope

@Rich-Harris
Copy link
Member

Here's a workaround in the meantime, pending a more complete decision:

// scripts/vercel-images.js
import fs from 'node:fs';

const file = '.vercel/output/config.json';

const config = {
  ...JSON.parse(fs.readFileSync(file, 'utf-8')),
  images: {
    // config goes here
  }
};

fs.writeFileSync(file, JSON.stringify(config, null, 2));
// package.json
{
  // ...
  "scripts": {
    // ...
    "build": "vite build && node scripts/vercel-images"
  }
}

@devunt
Copy link
Contributor

devunt commented Mar 30, 2023

When will this land? Are there anything that I can help? I need this desperately.

@hartwm
Copy link

hartwm commented Mar 30, 2023

When will this land? Are there anything that I can help? I need this desperately.

Check this repo for implementation
https://github.com/hartwm/vercel-images-sveltekit

@benmccann
Copy link
Member

Thank you for this. However, I'm going to go ahead and close it for the reasons Rich mentioned above. We do recognize this is important and took a stab at implementing it in #9787. That's been on hold after we using it led to further discussion amongst the team. We've been a bit distracted building Svelte 5 since then. I know it's hard not to have any built-in solutions yet, but we've always erred on the side of getting it right vs getting it soon

@benmccann benmccann closed this Aug 10, 2023
@benmccann benmccann reopened this Oct 9, 2023
@benmccann
Copy link
Member

I personally think this PR makes sense to add now and we can follow up with getImage from #10323. Though the image optimization topic is pretty complex and will need discussion from the other maintainers. In the meantime, I have rebased this PR against master. There were several merge conflicts, so please test if it's still working

Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

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

this will still need some further discussion with the other maintainers as to whether we want to go in this direction, but this PR is well done and looks good to me

@eltigerchino eltigerchino added the needs-decision Not sure if we want to do this yet, also design work needed label Dec 3, 2023
@benmccann benmccann added this to the 2.0 milestone Dec 11, 2023
@benmccann benmccann removed the needs-decision Not sure if we want to do this yet, also design work needed label Dec 12, 2023
@benmccann benmccann changed the base branch from master to version-2 December 12, 2023 20:11
@benmccann benmccann merged commit 85e32d9 into sveltejs:version-2 Dec 12, 2023
11 checks passed
@github-actions github-actions bot mentioned this pull request Dec 14, 2023
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.

Adapter Vercel Image Optimizations
7 participants