Skip to content

Commit fbdc10f

Browse files
PrincesseuhPrincesseuh
authored
fix(assets): Fixes assets generation when using custom paths and configs (#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
1 parent 498866c commit fbdc10f

File tree

15 files changed

+272
-32
lines changed

15 files changed

+272
-32
lines changed

‎.changeset/few-avocados-notice.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"astro": patch
3+
---
4+
5+
Fixes `astro:assets` not working when using complex config with `vite.build.rollupOptions.output.assetFileNames`

‎packages/astro/e2e/dev-toolbar-audits.test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ test.describe('Dev Toolbar - Audits', () => {
4444
await appButton.click();
4545
});
4646

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

5050
const toolbar = page.locator('astro-dev-toolbar');

‎packages/astro/src/assets/build/generate.ts

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import fs, { readFileSync } from 'node:fs';
2-
import { basename, join } from 'node:path/posix';
2+
import { basename } from 'node:path/posix';
33
import { dim, green } from 'kleur/colors';
44
import type PQueue from 'p-queue';
55
import type { AstroConfig } from '../../@types/astro.js';
@@ -9,7 +9,7 @@ import { getTimeStat } from '../../core/build/util.js';
99
import { AstroError } from '../../core/errors/errors.js';
1010
import { AstroErrorData } from '../../core/errors/index.js';
1111
import type { Logger } from '../../core/logger/core.js';
12-
import { isRemotePath, prependForwardSlash } from '../../core/path.js';
12+
import { isRemotePath, removeLeadingForwardSlash } from '../../core/path.js';
1313
import { isServerLikeOutput } from '../../prerender/utils.js';
1414
import type { MapValue } from '../../type-utils.js';
1515
import { getConfiguredImageService } from '../internal.js';
@@ -89,10 +89,7 @@ export async function prepareAssetsGenerationEnv(
8989
}
9090

9191
function getFullImagePath(originalFilePath: string, env: AssetEnv): URL {
92-
return new URL(
93-
'.' + prependForwardSlash(join(env.assetsFolder, basename(originalFilePath))),
94-
env.serverRoot
95-
);
92+
return new URL(removeLeadingForwardSlash(originalFilePath), env.serverRoot);
9693
}
9794

9895
export async function generateImagesForPath(
@@ -115,7 +112,7 @@ export async function generateImagesForPath(
115112
// For instance, the same image could be referenced in both a server-rendered page and build-time-rendered page
116113
if (
117114
!env.isSSR &&
118-
!isRemotePath(originalFilePath) &&
115+
transformsAndPath.originalSrcPath &&
119116
!globalThis.astroAsset.referencedImages?.has(transformsAndPath.originalSrcPath)
120117
) {
121118
try {

‎packages/astro/src/assets/internal.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ export async function getImage(
7474
}
7575
}
7676

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

8181
// Clone the `src` object if it's an ESM import so that we don't refer to any properties of the original object
8282
// Causing our generate step to think the image is used outside of the image optimization pipeline
@@ -112,10 +112,14 @@ export async function getImage(
112112
!(isRemoteImage(validatedOptions.src) && imageURL === validatedOptions.src)
113113
) {
114114
const propsToHash = service.propertiesToHash ?? DEFAULT_HASH_PROPS;
115-
imageURL = globalThis.astroAsset.addStaticImage(validatedOptions, propsToHash, originalPath);
115+
imageURL = globalThis.astroAsset.addStaticImage(
116+
validatedOptions,
117+
propsToHash,
118+
originalFilePath
119+
);
116120
srcSets = srcSetTransforms.map((srcSet) => ({
117121
transform: srcSet.transform,
118-
url: globalThis.astroAsset.addStaticImage!(srcSet.transform, propsToHash, originalPath),
122+
url: globalThis.astroAsset.addStaticImage!(srcSet.transform, propsToHash, originalFilePath),
119123
descriptor: srcSet.descriptor,
120124
attributes: srcSet.attributes,
121125
}));

‎packages/astro/src/assets/types.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ export type ImageOutputFormat = (typeof VALID_OUTPUT_FORMATS)[number] | (string
1111
export type AssetsGlobalStaticImagesList = Map<
1212
string,
1313
{
14-
originalSrcPath: string;
14+
originalSrcPath: string | undefined;
1515
transforms: Map<string, { finalPath: string; transform: ImageTransform }>;
1616
}
1717
>;
@@ -21,7 +21,7 @@ declare global {
2121
var astroAsset: {
2222
imageService?: ImageService;
2323
addStaticImage?:
24-
| ((options: ImageTransform, hashProperties: string[], fsPath: string) => string)
24+
| ((options: ImageTransform, hashProperties: string[], fsPath: string | undefined) => string)
2525
| undefined;
2626
staticImages?: AssetsGlobalStaticImagesList;
2727
referencedImages?: Set<string>;

‎packages/astro/src/assets/utils/transformToPath.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,18 @@
1-
import { basename, extname } from 'node:path';
1+
import { basename, dirname, extname } from 'node:path';
22
import { deterministicString } from 'deterministic-object-hash';
33
import { removeQueryString } from '../../core/path.js';
44
import { shorthash } from '../../runtime/server/shorthash.js';
55
import type { ImageTransform } from '../types.js';
66
import { isESMImportedImage } from './imageKind.js';
77

8-
export function propsToFilename(transform: ImageTransform, hash: string) {
9-
let filename = removeQueryString(
10-
isESMImportedImage(transform.src) ? transform.src.src : transform.src
11-
);
8+
export function propsToFilename(filePath: string, transform: ImageTransform, hash: string) {
9+
let filename = decodeURIComponent(removeQueryString(filePath));
1210
const ext = extname(filename);
13-
filename = decodeURIComponent(basename(filename, ext));
11+
filename = basename(filename, ext);
12+
const prefixDirname = isESMImportedImage(transform.src) ? dirname(filePath) : '';
1413

1514
let outputExt = transform.format ? `.${transform.format}` : ext;
16-
return `/${filename}_${hash}${outputExt}`;
15+
return decodeURIComponent(`${prefixDirname}/${filename}_${hash}${outputExt}`);
1716
}
1817

1918
export function hashTransform(

‎packages/astro/src/assets/vite-plugin-assets.ts

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
appendForwardSlash,
1010
joinPaths,
1111
prependForwardSlash,
12+
removeBase,
1213
removeQueryString,
1314
} from '../core/path.js';
1415
import { isServerLikeOutput } from '../prerender/utils.js';
@@ -91,7 +92,7 @@ export default function assets({
9192
return;
9293
}
9394

94-
globalThis.astroAsset.addStaticImage = (options, hashProperties, originalPath) => {
95+
globalThis.astroAsset.addStaticImage = (options, hashProperties, originalFSPath) => {
9596
if (!globalThis.astroAsset.staticImages) {
9697
globalThis.astroAsset.staticImages = new Map<
9798
string,
@@ -102,13 +103,18 @@ export default function assets({
102103
>();
103104
}
104105

105-
// Rollup will copy the file to the output directory, this refer to this final path, not to the original path
106+
// Rollup will copy the file to the output directory, as such this is the path in the output directory, including the asset prefix / base
106107
const ESMImportedImageSrc = isESMImportedImage(options.src)
107108
? options.src.src
108109
: options.src;
109110
const fileExtension = extname(ESMImportedImageSrc);
110-
const pf = getAssetsPrefix(fileExtension, settings.config.build.assetsPrefix);
111-
const finalOriginalImagePath = ESMImportedImageSrc.replace(pf, '');
111+
const assetPrefix = getAssetsPrefix(fileExtension, settings.config.build.assetsPrefix);
112+
113+
// 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)
114+
const finalOriginalPath = removeBase(
115+
removeBase(ESMImportedImageSrc, settings.config.base),
116+
assetPrefix
117+
);
112118

113119
const hash = hashTransform(
114120
options,
@@ -117,21 +123,26 @@ export default function assets({
117123
);
118124

119125
let finalFilePath: string;
120-
let transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalImagePath);
126+
let transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalPath);
121127
let transformForHash = transformsForPath?.transforms.get(hash);
128+
129+
// If the same image has already been transformed with the same options, we'll reuse the final path
122130
if (transformsForPath && transformForHash) {
123131
finalFilePath = transformForHash.finalPath;
124132
} else {
125133
finalFilePath = prependForwardSlash(
126-
joinPaths(settings.config.build.assets, propsToFilename(options, hash))
134+
joinPaths(
135+
isESMImportedImage(options.src) ? '' : settings.config.build.assets,
136+
prependForwardSlash(propsToFilename(finalOriginalPath, options, hash))
137+
)
127138
);
128139

129140
if (!transformsForPath) {
130-
globalThis.astroAsset.staticImages.set(finalOriginalImagePath, {
131-
originalSrcPath: originalPath,
141+
globalThis.astroAsset.staticImages.set(finalOriginalPath, {
142+
originalSrcPath: originalFSPath,
132143
transforms: new Map(),
133144
});
134-
transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalImagePath)!;
145+
transformsForPath = globalThis.astroAsset.staticImages.get(finalOriginalPath)!;
135146
}
136147

137148
transformsForPath.transforms.set(hash, {
@@ -143,7 +154,7 @@ export default function assets({
143154
// The paths here are used for URLs, so we need to make sure they have the proper format for an URL
144155
// (leading slash, prefixed with the base / assets prefix, encoded, etc)
145156
if (settings.config.build.assetsPrefix) {
146-
return encodeURI(joinPaths(pf, finalFilePath));
157+
return encodeURI(joinPaths(assetPrefix, finalFilePath));
147158
} else {
148159
return encodeURI(prependForwardSlash(joinPaths(settings.config.base, finalFilePath)));
149160
}

‎packages/astro/test/core-image-unconventional-settings.test.js

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,187 @@
1+
import assert from 'node:assert/strict';
2+
import { describe, it } from 'node:test';
3+
import * as cheerio from 'cheerio';
4+
5+
import { testImageService } from './test-image-service.js';
6+
import { loadFixture } from './test-utils.js';
7+
8+
/**
9+
** @typedef {import('../src/@types/astro').AstroInlineConfig & { root?: string | URL }} AstroInlineConfig
10+
*/
11+
12+
/** @type {AstroInlineConfig} */
13+
const defaultSettings = {
14+
root: './fixtures/core-image-unconventional-settings/',
15+
image: {
16+
service: testImageService(),
17+
},
18+
};
19+
20+
describe('astro:assets - Support unconventional build settings properly', () => {
21+
/** @type {import('./test-utils').Fixture} */
22+
let fixture;
23+
24+
it('supports assetsPrefix', async () => {
25+
fixture = await loadFixture({
26+
...defaultSettings,
27+
build: {
28+
assetsPrefix: 'https://cdn.example.com/',
29+
},
30+
});
31+
await fixture.build();
32+
33+
const html = await fixture.readFile('/index.html');
34+
const $ = cheerio.load(html);
35+
const src = $('#walrus-img').attr('src');
36+
assert.equal(src.startsWith('https://cdn.example.com/'), true);
37+
38+
const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
39+
assert.equal(data instanceof Buffer, true);
40+
});
41+
42+
it('supports base', async () => {
43+
fixture = await loadFixture({
44+
...defaultSettings,
45+
build: {
46+
base: '/subdir/',
47+
},
48+
});
49+
await fixture.build();
50+
51+
const html = await fixture.readFile('/index.html');
52+
const $ = cheerio.load(html);
53+
const src = $('#walrus-img').attr('src');
54+
const data = await fixture.readFile(src.replace('/subdir/', ''), null);
55+
assert.equal(data instanceof Buffer, true);
56+
});
57+
58+
// This test is a bit of a stretch, but it's a good sanity check, `assetsPrefix` should take precedence over `base` in this context
59+
it('supports assetsPrefix + base', async () => {
60+
fixture = await loadFixture({
61+
...defaultSettings,
62+
build: {
63+
assetsPrefix: 'https://cdn.example.com/',
64+
base: '/subdir/',
65+
},
66+
});
67+
await fixture.build();
68+
69+
const html = await fixture.readFile('/index.html');
70+
const $ = cheerio.load(html);
71+
const src = $('#walrus-img').attr('src');
72+
assert.equal(src.startsWith('https://cdn.example.com/'), true);
73+
74+
const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
75+
assert.equal(data instanceof Buffer, true);
76+
});
77+
78+
it('supports custom build.assets', async () => {
79+
fixture = await loadFixture({
80+
...defaultSettings,
81+
build: {
82+
assets: 'assets',
83+
},
84+
});
85+
await fixture.build();
86+
87+
const html = await fixture.readFile('/index.html');
88+
const $ = cheerio.load(html);
89+
90+
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
91+
assert.equal(unoptimizedSrc.startsWith('/assets/'), true);
92+
93+
const src = $('#walrus-img').attr('src');
94+
const data = await fixture.readFile(src, null);
95+
96+
assert.equal(data instanceof Buffer, true);
97+
});
98+
99+
it('supports custom vite.build.rollupOptions.output.assetFileNames', async () => {
100+
fixture = await loadFixture({
101+
...defaultSettings,
102+
vite: {
103+
build: {
104+
rollupOptions: {
105+
output: {
106+
assetFileNames: 'images/hello_[name].[ext]',
107+
},
108+
},
109+
},
110+
},
111+
});
112+
await fixture.build();
113+
114+
const html = await fixture.readFile('/index.html');
115+
const $ = cheerio.load(html);
116+
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
117+
assert.equal(unoptimizedSrc, '/images/hello_light_walrus.avif');
118+
119+
const src = $('#walrus-img').attr('src');
120+
const data = await fixture.readFile(src, null);
121+
122+
assert.equal(data instanceof Buffer, true);
123+
});
124+
125+
it('supports complex vite.build.rollupOptions.output.assetFileNames', async () => {
126+
fixture = await loadFixture({
127+
...defaultSettings,
128+
vite: {
129+
build: {
130+
rollupOptions: {
131+
output: {
132+
assetFileNames: 'assets/[hash]/[name][extname]',
133+
},
134+
},
135+
},
136+
},
137+
});
138+
await fixture.build();
139+
140+
const html = await fixture.readFile('/index.html');
141+
const $ = cheerio.load(html);
142+
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
143+
const originalData = await fixture.readFile(unoptimizedSrc, null);
144+
assert.equal(originalData instanceof Buffer, true);
145+
146+
const src = $('#walrus-img').attr('src');
147+
const data = await fixture.readFile(src, null);
148+
149+
assert.equal(data instanceof Buffer, true);
150+
});
151+
152+
it('supports custom vite.build.rollupOptions.output.assetFileNames with assetsPrefix', async () => {
153+
fixture = await loadFixture({
154+
...defaultSettings,
155+
vite: {
156+
build: {
157+
rollupOptions: {
158+
output: {
159+
assetFileNames: 'images/hello_[name].[ext]',
160+
},
161+
},
162+
},
163+
},
164+
build: {
165+
assetsPrefix: 'https://cdn.example.com/',
166+
},
167+
});
168+
await fixture.build();
169+
170+
const html = await fixture.readFile('/index.html');
171+
const $ = cheerio.load(html);
172+
const unoptimizedSrc = $('#walrus-img-unoptimized').attr('src');
173+
assert.equal(unoptimizedSrc, 'https://cdn.example.com/images/hello_light_walrus.avif');
174+
175+
const unoptimizedData = await fixture.readFile(
176+
unoptimizedSrc.replace('https://cdn.example.com/', ''),
177+
null
178+
);
179+
assert.equal(unoptimizedData instanceof Buffer, true);
180+
181+
const src = $('#walrus-img').attr('src');
182+
assert.equal(src.startsWith('https://cdn.example.com/'), true);
183+
184+
const data = await fixture.readFile(src.replace('https://cdn.example.com/', ''), null);
185+
assert.equal(data instanceof Buffer, true);
186+
});
187+
});

0 commit comments

Comments
 (0)