Skip to content

Commit

Permalink
fix(assets): Fixes assets generation when using custom paths and conf…
Browse files Browse the repository at this point in the history
…igs (#10567)

* fix(assets): Fixes assets generation when using custom paths and configs

* fix: tests

* fix: make sure remote files don't end up in nested folders by accident

* test: add a whole bunch of tests

* chore: changeset
  • Loading branch information
Princesseuh authored Mar 28, 2024
1 parent 498866c commit fbdc10f
Show file tree
Hide file tree
Showing 15 changed files with 272 additions and 32 deletions.
5 changes: 5 additions & 0 deletions .changeset/few-avocados-notice.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes `astro:assets` not working when using complex config with `vite.build.rollupOptions.output.assetFileNames`
2 changes: 1 addition & 1 deletion packages/astro/e2e/dev-toolbar-audits.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ test.describe('Dev Toolbar - Audits', () => {
await appButton.click();
});

test('can handle mutations zzz', async ({ page, astro }) => {
test('can handle mutations', async ({ page, astro }) => {
await page.goto(astro.resolveUrl('/audits-mutations'));

const toolbar = page.locator('astro-dev-toolbar');
Expand Down
11 changes: 4 additions & 7 deletions packages/astro/src/assets/build/generate.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import fs, { readFileSync } from 'node:fs';
import { basename, join } from 'node:path/posix';
import { basename } from 'node:path/posix';
import { dim, green } from 'kleur/colors';
import type PQueue from 'p-queue';
import type { AstroConfig } from '../../@types/astro.js';
Expand All @@ -9,7 +9,7 @@ import { getTimeStat } from '../../core/build/util.js';
import { AstroError } from '../../core/errors/errors.js';
import { AstroErrorData } from '../../core/errors/index.js';
import type { Logger } from '../../core/logger/core.js';
import { isRemotePath, prependForwardSlash } from '../../core/path.js';
import { isRemotePath, removeLeadingForwardSlash } from '../../core/path.js';
import { isServerLikeOutput } from '../../prerender/utils.js';
import type { MapValue } from '../../type-utils.js';
import { getConfiguredImageService } from '../internal.js';
Expand Down Expand Up @@ -89,10 +89,7 @@ export async function prepareAssetsGenerationEnv(
}

function getFullImagePath(originalFilePath: string, env: AssetEnv): URL {
return new URL(
'.' + prependForwardSlash(join(env.assetsFolder, basename(originalFilePath))),
env.serverRoot
);
return new URL(removeLeadingForwardSlash(originalFilePath), env.serverRoot);
}

export async function generateImagesForPath(
Expand All @@ -115,7 +112,7 @@ export async function generateImagesForPath(
// For instance, the same image could be referenced in both a server-rendered page and build-time-rendered page
if (
!env.isSSR &&
!isRemotePath(originalFilePath) &&
transformsAndPath.originalSrcPath &&
!globalThis.astroAsset.referencedImages?.has(transformsAndPath.originalSrcPath)
) {
try {
Expand Down
12 changes: 8 additions & 4 deletions packages/astro/src/assets/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,9 +74,9 @@ export async function getImage(
}
}

const originalPath = isESMImportedImage(resolvedOptions.src)
const originalFilePath = isESMImportedImage(resolvedOptions.src)
? resolvedOptions.src.fsPath
: resolvedOptions.src;
: undefined; // Only set for ESM imports, where we do have a file path

// Clone the `src` object if it's an ESM import so that we don't refer to any properties of the original object
// Causing our generate step to think the image is used outside of the image optimization pipeline
Expand Down Expand Up @@ -112,10 +112,14 @@ export async function getImage(
!(isRemoteImage(validatedOptions.src) && imageURL === validatedOptions.src)
) {
const propsToHash = service.propertiesToHash ?? DEFAULT_HASH_PROPS;
imageURL = globalThis.astroAsset.addStaticImage(validatedOptions, propsToHash, originalPath);
imageURL = globalThis.astroAsset.addStaticImage(
validatedOptions,
propsToHash,
originalFilePath
);
srcSets = srcSetTransforms.map((srcSet) => ({
transform: srcSet.transform,
url: globalThis.astroAsset.addStaticImage!(srcSet.transform, propsToHash, originalPath),
url: globalThis.astroAsset.addStaticImage!(srcSet.transform, propsToHash, originalFilePath),
descriptor: srcSet.descriptor,
attributes: srcSet.attributes,
}));
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/src/assets/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export type ImageOutputFormat = (typeof VALID_OUTPUT_FORMATS)[number] | (string
export type AssetsGlobalStaticImagesList = Map<
string,
{
originalSrcPath: string;
originalSrcPath: string | undefined;
transforms: Map<string, { finalPath: string; transform: ImageTransform }>;
}
>;
Expand All @@ -21,7 +21,7 @@ declare global {
var astroAsset: {
imageService?: ImageService;
addStaticImage?:
| ((options: ImageTransform, hashProperties: string[], fsPath: string) => string)
| ((options: ImageTransform, hashProperties: string[], fsPath: string | undefined) => string)
| undefined;
staticImages?: AssetsGlobalStaticImagesList;
referencedImages?: Set<string>;
Expand Down
13 changes: 6 additions & 7 deletions packages/astro/src/assets/utils/transformToPath.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,18 @@
import { basename, extname } from 'node:path';
import { basename, dirname, extname } from 'node:path';
import { deterministicString } from 'deterministic-object-hash';
import { removeQueryString } from '../../core/path.js';
import { shorthash } from '../../runtime/server/shorthash.js';
import type { ImageTransform } from '../types.js';
import { isESMImportedImage } from './imageKind.js';

export function propsToFilename(transform: ImageTransform, hash: string) {
let filename = removeQueryString(
isESMImportedImage(transform.src) ? transform.src.src : transform.src
);
export function propsToFilename(filePath: string, transform: ImageTransform, hash: string) {
let filename = decodeURIComponent(removeQueryString(filePath));
const ext = extname(filename);
filename = decodeURIComponent(basename(filename, ext));
filename = basename(filename, ext);
const prefixDirname = isESMImportedImage(transform.src) ? dirname(filePath) : '';

let outputExt = transform.format ? `.${transform.format}` : ext;
return `/${filename}_${hash}${outputExt}`;
return decodeURIComponent(`${prefixDirname}/${filename}_${hash}${outputExt}`);
}

export function hashTransform(
Expand Down
31 changes: 21 additions & 10 deletions packages/astro/src/assets/vite-plugin-assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
appendForwardSlash,
joinPaths,
prependForwardSlash,
removeBase,
removeQueryString,
} from '../core/path.js';
import { isServerLikeOutput } from '../prerender/utils.js';
Expand Down Expand Up @@ -91,7 +92,7 @@ export default function assets({
return;
}

globalThis.astroAsset.addStaticImage = (options, hashProperties, originalPath) => {
globalThis.astroAsset.addStaticImage = (options, hashProperties, originalFSPath) => {
if (!globalThis.astroAsset.staticImages) {
globalThis.astroAsset.staticImages = new Map<
string,
Expand All @@ -102,13 +103,18 @@ export default function assets({
>();
}

// Rollup will copy the file to the output directory, this refer to this final path, not to the original path
// Rollup will copy the file to the output directory, as such this is the path in the output directory, including the asset prefix / base
const ESMImportedImageSrc = isESMImportedImage(options.src)
? options.src.src
: options.src;
const fileExtension = extname(ESMImportedImageSrc);
const pf = getAssetsPrefix(fileExtension, settings.config.build.assetsPrefix);
const finalOriginalImagePath = ESMImportedImageSrc.replace(pf, '');
const assetPrefix = getAssetsPrefix(fileExtension, settings.config.build.assetsPrefix);

// This is the path to the original image, from the dist root, without the base or the asset prefix (e.g. /_astro/image.hash.png)
const finalOriginalPath = removeBase(
removeBase(ESMImportedImageSrc, settings.config.base),
assetPrefix
);

const hash = hashTransform(
options,
Expand All @@ -117,21 +123,26 @@ export default function assets({
);

let finalFilePath: string;
let transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalImagePath);
let transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalPath);
let transformForHash = transformsForPath?.transforms.get(hash);

// If the same image has already been transformed with the same options, we'll reuse the final path
if (transformsForPath && transformForHash) {
finalFilePath = transformForHash.finalPath;
} else {
finalFilePath = prependForwardSlash(
joinPaths(settings.config.build.assets, propsToFilename(options, hash))
joinPaths(
isESMImportedImage(options.src) ? '' : settings.config.build.assets,
prependForwardSlash(propsToFilename(finalOriginalPath, options, hash))
)
);

if (!transformsForPath) {
globalThis.astroAsset.staticImages.set(finalOriginalImagePath, {
originalSrcPath: originalPath,
globalThis.astroAsset.staticImages.set(finalOriginalPath, {
originalSrcPath: originalFSPath,
transforms: new Map(),
});
transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalImagePath)!;
transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalPath)!;
}

transformsForPath.transforms.set(hash, {
Expand All @@ -143,7 +154,7 @@ export default function assets({
// The paths here are used for URLs, so we need to make sure they have the proper format for an URL
// (leading slash, prefixed with the base / assets prefix, encoded, etc)
if (settings.config.build.assetsPrefix) {
return encodeURI(joinPaths(pf, finalFilePath));
return encodeURI(joinPaths(assetPrefix, finalFilePath));
} else {
return encodeURI(prependForwardSlash(joinPaths(settings.config.base, finalFilePath)));
}
Expand Down
187 changes: 187 additions & 0 deletions packages/astro/test/core-image-unconventional-settings.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
import assert from 'node:assert/strict';
import { describe, it } from 'node:test';
import * as cheerio from 'cheerio';

import { testImageService } from './test-image-service.js';
import { loadFixture } from './test-utils.js';

/**
** @typedef {import('../src/@types/astro').AstroInlineConfig & { root?: string | URL }} AstroInlineConfig
*/

/** @type {AstroInlineConfig} */
const defaultSettings = {
root: './fixtures/core-image-unconventional-settings/',
image: {
service: testImageService(),
},
};

describe('astro:assets - Support unconventional build settings properly', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;

it('supports assetsPrefix', async () => {
fixture = await loadFixture({
...defaultSettings,
build: {
assetsPrefix: 'https://cdn.example.com/',
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const src = $('#walrus-img').attr('src');
assert.equal(src.startsWith('https://cdn.example.com/'), true);

const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
assert.equal(data instanceof Buffer, true);
});

it('supports base', async () => {
fixture = await loadFixture({
...defaultSettings,
build: {
base: '/subdir/',
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const src = $('#walrus-img').attr('src');
const data = await fixture.readFile(src.replace('/subdir/', ''), null);
assert.equal(data instanceof Buffer, true);
});

// This test is a bit of a stretch, but it's a good sanity check, `assetsPrefix` should take precedence over `base` in this context
it('supports assetsPrefix + base', async () => {
fixture = await loadFixture({
...defaultSettings,
build: {
assetsPrefix: 'https://cdn.example.com/',
base: '/subdir/',
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const src = $('#walrus-img').attr('src');
assert.equal(src.startsWith('https://cdn.example.com/'), true);

const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
assert.equal(data instanceof Buffer, true);
});

it('supports custom build.assets', async () => {
fixture = await loadFixture({
...defaultSettings,
build: {
assets: 'assets',
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);

const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
assert.equal(unoptimizedSrc.startsWith('/assets/'), true);

const src = $('#walrus-img').attr('src');
const data = await fixture.readFile(src, null);

assert.equal(data instanceof Buffer, true);
});

it('supports custom vite.build.rollupOptions.output.assetFileNames', async () => {
fixture = await loadFixture({
...defaultSettings,
vite: {
build: {
rollupOptions: {
output: {
assetFileNames: 'images/hello_[name].[ext]',
},
},
},
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
assert.equal(unoptimizedSrc, '/images/hello_light_walrus.avif');

const src = $('#walrus-img').attr('src');
const data = await fixture.readFile(src, null);

assert.equal(data instanceof Buffer, true);
});

it('supports complex vite.build.rollupOptions.output.assetFileNames', async () => {
fixture = await loadFixture({
...defaultSettings,
vite: {
build: {
rollupOptions: {
output: {
assetFileNames: 'assets/[hash]/[name][extname]',
},
},
},
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
const originalData = await fixture.readFile(unoptimizedSrc, null);
assert.equal(originalData instanceof Buffer, true);

const src = $('#walrus-img').attr('src');
const data = await fixture.readFile(src, null);

assert.equal(data instanceof Buffer, true);
});

it('supports custom vite.build.rollupOptions.output.assetFileNames with assetsPrefix', async () => {
fixture = await loadFixture({
...defaultSettings,
vite: {
build: {
rollupOptions: {
output: {
assetFileNames: 'images/hello_[name].[ext]',
},
},
},
},
build: {
assetsPrefix: 'https://cdn.example.com/',
},
});
await fixture.build();

const html = await fixture.readFile('/index.html');
const $ = cheerio.load(html);
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
assert.equal(unoptimizedSrc, 'https://cdn.example.com/images/hello_light_walrus.avif');

const unoptimizedData = await fixture.readFile(
unoptimizedSrc.replace('https://cdn.example.com/', ''),
null
);
assert.equal(unoptimizedData instanceof Buffer, true);

const src = $('#walrus-img').attr('src');
assert.equal(src.startsWith('https://cdn.example.com/'), true);

const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
assert.equal(data instanceof Buffer, true);
});
});
Loading

0 comments on commit fbdc10f

Please sign in to comment.