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

WIP: [image] Fixing SSR support and improving error validation #4013

Merged
merged 12 commits into from
Jul 22, 2022

Conversation

tony-sull
Copy link
Contributor

@tony-sull tony-sull commented Jul 21, 2022

Changes

  • fixes an undefined error hit in SSR deployments (related to the use of globalThis)
  • ☝️ uncovered an issue that original image files in src weren't being copied to dist for SSR builds
  • Follow-up to Handle EXIF orientation flag #4021, ensures sharp rotates images based on EXIF data
  • cleans up the project structure

Testing

  • re-enables a couple SSR tests that would have found the broken SSR support
  • adds tests for sharp's image rotation based on EXIF data

Docs

Stay tuned! A follow-up PR will include README updates with much more detail on the built-in components

@changeset-bot
Copy link

changeset-bot bot commented Jul 21, 2022

🦋 Changeset detected

Latest commit: 56ed3e2

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

This PR includes changesets to release 1 package
Name Type
@astrojs/image 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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jul 21, 2022
@tony-sull tony-sull marked this pull request as ready for review July 22, 2022 21:10
@tony-sull tony-sull marked this pull request as draft July 22, 2022 21:10
import { isRemoteImage, loadRemoteImage, loadLocalImage } from '../utils/images.js';
import type { SSRImageService, TransformOptions } from '../types.js';

export interface SSGBuildParams {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

build step for generating SSG images moved out of the main integration

}

export async function ssrBuild({ srcDir, outDir }: SSRBuildParams) {
const images = await globImages(srcDir);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SSR build step moved out of the main integration

This was actually a bug uncovered while fixing the undefined error, original images weren't being copied to dist for SSR


const inputBuffer = await loadRemoteImage(href.toString());
if (isRemoteImage(transform.src)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

in SSR, looking up the original files that were copied to dist during the build

@@ -1,14 +1,15 @@
import slash from 'slash';
import { ROUTE_PATTERN } from './constants.js';
import { ROUTE_PATTERN } from '../constants.js';
import sharp from '../loaders/sharp.js';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TEMP: avoiding another globalThis here by depending on the sharp import

When we add squoosh, this will import from a helper that checks if sharp is installed first


if (!loader) {
// @ts-ignore
const { default: mod } = await import('virtual:image-loader');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lazy importing the loader so it's only imported if/when the getImage() helper is used

@@ -84,6 +84,9 @@ class SharpService implements SSRImageService {
async transform(inputBuffer: Buffer, transform: TransformOptions) {
const sharpImage = sharp(inputBuffer, { failOnError: false });

// always call rotate to adjust for EXIF data orientation
sharpImage.rotate();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up to #4021, sharp needs to rotate the image based on EXIF data

@tony-sull tony-sull marked this pull request as ready for review July 22, 2022 21:54
Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

Looks great!

@astrobot-houston astrobot-houston mentioned this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
2 participants