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: Astro Assets via Sharp by default #3493

Merged
merged 25 commits into from
Dec 6, 2023
Merged

Conversation

nerdoza
Copy link

@nerdoza nerdoza commented Nov 6, 2023

This PR adds the automatic creation of a Sharp layer for AstroSite deployments using Sharp for image processing. As part of this update, the squoosh compatibility has been dropped from the adapter as there is not a readily available Squoosh layer pakcage.

This PR also fixes the issue of the adapter export (aws()) not having types due to the types.d.ts definition file not being included in the dist output.

All changes in this PR should be backwards compatible and all configurations can be overwritten.

Copy link

changeset-bot bot commented Nov 6, 2023

🦋 Changeset detected

Latest commit: fb381b3

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

This PR includes changesets to release 6 packages
Name Type
astro-sst Patch
sst Patch
@sst/console Patch
create-sst Patch
svelte-kit-sst Patch
solid-start-sst Patch

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

@nerdoza
Copy link
Author

nerdoza commented Nov 8, 2023

This PR has wondered through the capability space and has finally settled on adopting a more dynamic and DX friendly approach with an easy "advanced" user path via the docs.

  • Adds support for sharp image process for regional and edge deployments without any config changes.
  • Adds docs for how to add a layer to regional deployments for a more optimized runtime.
  • Adds architecture as a SsrSite prop with arm_64 as the default architecture for SSR functions.
  • Removes squoosh as a supported asset optimizer from the adapter.

@nerdoza nerdoza requested a review from fwang November 9, 2023 21:20
@fwang
Copy link
Contributor

fwang commented Nov 13, 2023

@bayssmekanique oops I forgot to send this after typing it up

From what I gathered so far (I might be wrong), 3 ways to get Sharp to work:

  1. Use the bundled version as in: with ur fix to respect the architecture, the bundled version should just work. Is the downside having to npm i sharp on every deploy?
  2. Bundle a pre-existing version of sharp from astro-sst as a node_modules folder. Is the downside having to upload this node_modules folder on every deploy?
  3. Bundle a pre-existing version of sharp from astro-sst as a layer. There were some thoughts in the OpenNext community that the image optimizer should be deployed to the edge. It is not right now b/c deploying an edge function is slow in CloudFront (takes minutes). But I want to keep this a possible by not using a layer. Also using layer doesn't actually reduce the cold start.

I'm leaning towards 1 or 2 more.

@nerdoza
Copy link
Author

nerdoza commented Nov 13, 2023

@fwang,

  1. Bundled version (with this fix): This is my preference for a default. It works for regional and edge deployment, doesn't introduce dependency locking, and has a clean path for overriding. The only downside to this is there is a lot of extra junk from the npm package that doesn't get used by the runtime but is still loaded into the environment. I believe npm should cache the dependency in the local cache so it shouldn't have to download on each rebuild (just on first build).

  2. Bundled...?: I'm not entirely sure how this differs from (1). Sounds like the same but with a bundled version in astro-sst. I would avoid doing anything that adds dependencies when not necessary as they will eventually cause headaches when they fall too far behind. That's also my thinking for resisting the layers approach.

  3. Bundling a pre-built layer: This was my initial implementation strategy and in theory it should be more performant, not because of anything intrinsic to the layer, but because the prebuilt layers have been stripped of all "fluff" so it makes the entire deployed code smaller. This, however, introduces a version dependency within the astro-sst which will require maintenance and it doesn't work for edge deployments. For these reasons, I think it makes more sense to treat this as the "advanced" strategy made available through the documentation but not a default configuration.

All in all, I think strategy (1) is the best approach, and that's basically what's represented in this PR. It works on the most environment and has the most maintainable path forward. Bundling in a version of sharp might be more performant from a build and possibly even runtime standing, but it will cause headaches eventually when there is a demand for an update to the bundled sharp version but the update will break backwards compatibility so you end up doing a major release just for a bundled dependency.

A few things I started to work on but decided against:
A) Making bundling capability to be a bit more dynamic so the packages to bundle could be defined at build time. Astro supports squoosh as an alternative to sharp, but making it automatically detect and bundle squoosh had a few pitfalls that didn't seem worth it.

B) Making a separate Lambda for asset paths with the sharp bundle, and another lighter Lambda for all other routes. This lead to me looking into the ImageOptimizer function which seems like it's meant to do exactly that, but the implementation seems to be specific to a framework (probably OpenNext?) so I didn't pursue that any further.

@@ -88,6 +87,7 @@ export default function createIntegration(
updateConfig({
output: "server",
});
config.output = "server";
Copy link
Author

Choose a reason for hiding this comment

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

FWIW, this is ugly and I have a PR with Astro open to make it so the config can be returned after an update to the config: withastro/astro#9013

Copy link
Author

Choose a reason for hiding this comment

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

This will be fixed once Astro v4 is released.

@fwang
Copy link
Contributor

fwang commented Nov 23, 2023

@bayssmekanique I'm a bit hesitate on adding a top level SsrSite.architecture prop, as it's not supported by L@E (as u mentioned in the comment). Currently, you can set SsrSite.cdk.server.architecture and that only applies to the server function. So I pushed a change to use SsrSite.cdk.server.architecture instead.

Let me know if it looks good, and I will merge it.

@nerdoza
Copy link
Author

nerdoza commented Nov 26, 2023

@fwang, I understand the concern, though I would expect that before long there will likely be support for alternate runtime architectures on L@E.

These reversions nullify the changes that fixed the asset handler. The issue is that without an architecture defined somewhere in the SsrFunction scope the bundler doesn't know which architecture to use and uses the default (which on my machine is x86_64) which doesn't necessarily match the default configuration used by Lambda (which at the moment is arm_64).

I've pushed another commit adding a top level default to the SsrFunction of ARM_64 while still allowing override. This will give the bundler something concrete to work off instead of hoping the defaults align.

Set a default architecture for SsrFunction so bundler can correctly bundle resources.
@jlarmstrongiv
Copy link

I just merged Astro _image endpoint support to unpic in ascorbic/unpic#94, which means as soon as this PR lands, we’ll have support for Astro assets in all ui frameworks (react, preact, svelte, vue, solidjs, lit, qwik) with SST. Combined, this will be a game changer 😄

@fwang fwang merged commit d5d31fd into sst:master Dec 6, 2023
1 check failed
@github-actions github-actions bot mentioned this pull request Dec 6, 2023
@nerdoza nerdoza deleted the feat/astro-assets branch December 6, 2023 17:53
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.

3 participants