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: support images outside the project #7139

Merged
merged 8 commits into from
May 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wise-cars-hear.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

The `src` property returned by ESM importing images with `astro:assets` is now an absolute path, unlocking support for importing images outside the project.
9 changes: 9 additions & 0 deletions packages/astro/src/assets/services/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ export type BaseServiceTransform = {
*/
export const baseService: Omit<LocalImageService, 'transform'> = {
validateOptions(options) {
// `src` is missing or is `undefined`.
if (!options.src || (typeof options.src !== 'string' && typeof options.src !== 'object')) {
throw new AstroError({
...AstroErrorData.ExpectedImage,
Expand All @@ -117,6 +118,14 @@ export const baseService: Omit<LocalImageService, 'transform'> = {
}

if (!isESMImportedImage(options.src)) {
// User passed an `/@fs/` path instead of the full image.
if (options.src.startsWith('/@fs/')) {
throw new AstroError({
...AstroErrorData.LocalImageUsedWrongly,
message: AstroErrorData.LocalImageUsedWrongly.message(options.src),
});
}

// For remote images, width and height are explicitly required as we can't infer them from the file
let missingDimension: 'width' | 'height' | 'both' | undefined;
if (!options.width && !options.height) {
Expand Down
27 changes: 3 additions & 24 deletions packages/astro/src/assets/utils/emitAsset.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ import fs from 'node:fs';
import path from 'node:path';
import { fileURLToPath, pathToFileURL } from 'node:url';
import slash from 'slash';
import type { AstroConfig, AstroSettings } from '../../@types/astro';
import { prependForwardSlash } from '../../core/path.js';
import { imageMetadata, type Metadata } from './metadata.js';

export async function emitESMImage(
id: string | undefined,
watchMode: boolean,
fileEmitter: any,
settings: Pick<AstroSettings, 'config'>
fileEmitter: any
): Promise<Metadata | undefined> {
if (!id) {
return undefined;
Expand Down Expand Up @@ -40,34 +39,14 @@ export async function emitESMImage(
url.searchParams.append('origHeight', meta.height.toString());
url.searchParams.append('origFormat', meta.format);

meta.src = rootRelativePath(settings.config, url);
meta.src = `/@fs` + prependForwardSlash(fileURLToNormalizedPath(url));
Comment on lines -43 to +42
Copy link
Member

Choose a reason for hiding this comment

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

A note on this: Vite alternates between a root relative path and /@fs depending on whether the path is inside the root or not. Here it's always using /@fs which is and should fine, but there are small bugs now and then because Vite's rule is not applied by frameworks. Something to keep an eye on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I'm willing to take the risk. I think the consistency of it is great, even though I don't necessarily want users to rely on it. Will definitely reconsider if it becomes an issue!

}

return meta;
}

/**
* Utilities inlined from `packages/astro/src/core/util.ts`
* Avoids ESM / CJS bundling failures when accessed from integrations
* due to Vite dependencies in core.
*/

function rootRelativePath(config: Pick<AstroConfig, 'root'>, url: URL): string {
const basePath = fileURLToNormalizedPath(url);
const rootPath = fileURLToNormalizedPath(config.root);
return prependForwardSlash(basePath.slice(rootPath.length));
}

function prependForwardSlash(filePath: string): string {
return filePath[0] === '/' ? filePath : '/' + filePath;
}

function fileURLToNormalizedPath(filePath: URL): string {
// Uses `slash` package instead of Vite's `normalizePath`
// to avoid CJS bundling issues.
return slash(fileURLToPath(filePath) + filePath.search).replace(/\\/g, '/');
}

export function emoji(char: string, fallback: string): string {
return process.platform !== 'win32' ? char : fallback;
}
9 changes: 4 additions & 5 deletions packages/astro/src/assets/vite-plugin-assets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,12 @@ export default function assets({
}

const url = new URL(req.url, 'file:');
const filePath = url.searchParams.get('href');

if (!filePath) {
if (!url.searchParams.has('href')) {
return next();
}

const filePathURL = new URL('.' + filePath, settings.config.root);
const filePath = url.searchParams.get('href')?.slice('/@fs'.length);
const filePathURL = new URL('.' + filePath, 'file:');
Princesseuh marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +114 to +115
Copy link
Member

Choose a reason for hiding this comment

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

This actually kinda neat and it seems to handle Windows too. Vite handles it in a slightly complex way.

const file = await fs.readFile(filePathURL);

// Get the file's metadata from the URL
Expand Down Expand Up @@ -243,7 +242,7 @@ export default function assets({

const cleanedUrl = removeQueryString(id);
if (/\.(jpeg|jpg|png|tiff|webp|gif|svg)$/.test(cleanedUrl)) {
const meta = await emitESMImage(id, this.meta.watchMode, this.emitFile, settings);
const meta = await emitESMImage(id, this.meta.watchMode, this.emitFile);
return `export default ${JSON.stringify(meta)}`;
}
},
Expand Down
10 changes: 2 additions & 8 deletions packages/astro/src/content/runtime-assets.ts
Original file line number Diff line number Diff line change
@@ -1,21 +1,15 @@
import type { PluginContext } from 'rollup';
import { z } from 'zod';
import type { AstroSettings } from '../@types/astro.js';
import { emitESMImage } from '../assets/index.js';

export function createImage(
settings: Pick<AstroSettings, 'config'>,
pluginContext: PluginContext,
entryFilePath: string
) {
export function createImage(pluginContext: PluginContext, entryFilePath: string) {
return () => {
return z.string().transform(async (imagePath, ctx) => {
const resolvedFilePath = (await pluginContext.resolve(imagePath, entryFilePath))?.id;
const metadata = await emitESMImage(
resolvedFilePath,
pluginContext.meta.watchMode,
pluginContext.emitFile,
settings
pluginContext.emitFile
);

if (!metadata) {
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/content/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export async function getEntryData(
}

schema = schema({
image: createImage({ config }, pluginContext, entry._internal.filePath),
image: createImage(pluginContext, entry._internal.filePath),
});
}

Expand Down
47 changes: 35 additions & 12 deletions packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
* For unsupported formats such as SVGs and GIFs, you may be able to use an `img` tag directly:
* ```astro
* ---
* import rocket from '../assets/images/rocket.svg'
* import rocket from '../assets/images/rocket.svg';
* ---
*
* <img src={rocket.src} width={rocket.width} height={rocket.height} alt="A rocketship in space." />
Expand Down Expand Up @@ -626,7 +626,7 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
* Making changes to the response, such as setting headers, cookies, and the status code cannot be done outside of page components.
*/
ResponseSentError: {
title: 'Unable to set response',
title: 'Unable to set response.',
code: 3030,
message: 'The response has already been sent to the browser and cannot be altered.',
},
Expand All @@ -646,7 +646,7 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
* ```
*/
MiddlewareNoDataOrNextCalled: {
title: "The middleware didn't return a response or call `next`",
title: "The middleware didn't return a response or call `next`.",
code: 3031,
message:
'The middleware needs to either return a `Response` object or call the `next` function.',
Expand All @@ -666,7 +666,7 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
* ```
*/
MiddlewareNotAResponse: {
title: 'The middleware returned something that is not a `Response` object',
title: 'The middleware returned something that is not a `Response` object.',
code: 3032,
message: 'Any data returned from middleware must be a valid `Response` object.',
},
Expand All @@ -687,12 +687,38 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
* ```
*/
LocalsNotAnObject: {
title: 'Value assigned to `locals` is not accepted',
title: 'Value assigned to `locals` is not accepted.',
code: 3033,
message:
'`locals` can only be assigned to an object. Other values like numbers, strings, etc. are not accepted.',
hint: 'If you tried to remove some information from the `locals` object, try to use `delete` or set the property to `undefined`.',
},
/**
* @docs
* @see
* - [Assets (Experimental)](https://docs.astro.build/en/guides/assets/)
* @description
* When using the default image services, `Image`'s and `getImage`'s `src` parameter must be either an imported image or an URL, it cannot be a filepath.
*
* ```astro
* ---
* import { Image } from "astro:assets";
* import myImage from "../my_image.png";
* ---
*
* <!-- GOOD: `src` is the full imported image. -->
* <Image src={myImage} alt="Cool image" />
*
* <!-- BAD: `src` is an image's `src` path instead of the full image. -->
* <Image src={myImage.src} alt="Cool image" />
* ```
*/
LocalImageUsedWrongly: {
title: 'ESM imported images must be passed as-is.',
code: 3034,
message: (imageFilePath: string) =>
`\`Image\`'s and \`getImage\`'s \`src\` parameter must be an imported image or an URL, it cannot be a filepath. Received \`${imageFilePath}\`.`,
},
// No headings here, that way Vite errors are merged with Astro ones in the docs, which makes more sense to users.
// Vite Errors - 4xxx
/**
Expand Down Expand Up @@ -961,7 +987,6 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
},
/**
* @docs
* @message A content collection schema should not contain `slug` since it is reserved for slug generation. Remove this from your `COLLECTION_NAME` collection schema.
* @see
* - [The reserved entry `slug` field](https://docs.astro.build/en/guides/content-collections/)
* @description
Expand All @@ -970,9 +995,8 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
ContentSchemaContainsSlugError: {
title: 'Content Schema should not contain `slug`.',
code: 9003,
message: (collection: string) => {
return `A content collection schema should not contain \`slug\` since it is reserved for slug generation. Remove this from your ${collection} collection schema.`;
},
message: (collectionName: string) =>
`A content collection schema should not contain \`slug\` since it is reserved for slug generation. Remove this from your ${collectionName} collection schema.`,
hint: 'See https://docs.astro.build/en/guides/content-collections/ for more on the `slug` field.',
},

Expand All @@ -985,9 +1009,8 @@ See https://docs.astro.build/en/guides/server-side-rendering/ for more informati
CollectionDoesNotExistError: {
title: 'Collection does not exist',
code: 9004,
message: (collection: string) => {
return `The collection **${collection}** does not exist. Ensure a collection directory with this name exists.`;
},
message: (collectionName: string) =>
`The collection **${collectionName}** does not exist. Ensure a collection directory with this name exists.`,
hint: 'See https://docs.astro.build/en/guides/content-collections/ for more on creating collections.',
},
/**
Expand Down
42 changes: 34 additions & 8 deletions packages/astro/test/core-image.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,32 @@ describe('astro:image', () => {
expect(logs).to.have.a.lengthOf(1);
expect(logs[0].message).to.contain('Received unsupported format');
});

it("errors when an ESM imported image's src is passed to Image/getImage instead of the full import ssss", async () => {
logs.length = 0;
let res = await fixture.fetch('/error-image-src-passed');
await res.text();

expect(logs).to.have.a.lengthOf(1);
expect(logs[0].message).to.contain('must be an imported image or an URL');
});

it('supports images from outside the project', async () => {
let res = await fixture.fetch('/outsideProject');
let html = await res.text();
$ = cheerio.load(html);

let $img = $('img');
expect($img).to.have.a.lengthOf(2);
expect(
$img.toArray().every((img) => {
return (
img.attribs['src'].startsWith('/@fs/') ||
img.attribs['src'].startsWith('/_image?href=%2F%40fs%2F')
);
})
).to.be.true;
});
});

describe('vite-isms', () => {
Expand Down Expand Up @@ -228,9 +254,9 @@ describe('astro:image', () => {
expect($img).to.have.a.lengthOf(1);

// Verbose test for the full URL to make sure the image went through the full pipeline
expect($img.attr('src')).to.equal(
'/_image?href=%2Fsrc%2Fassets%2Fpenguin1.jpg%3ForigWidth%3D207%26origHeight%3D243%26origFormat%3Djpg&f=webp'
);
expect(
$img.attr('src').startsWith('/_image') && $img.attr('src').endsWith('f=webp')
).to.equal(true);
});

it('has width and height attributes', () => {
Expand Down Expand Up @@ -297,20 +323,20 @@ describe('astro:image', () => {

it('has proper source for directly used image', () => {
let $img = $('#direct-image img');
expect($img.attr('src').startsWith('/src/')).to.equal(true);
expect($img.attr('src').startsWith('/')).to.equal(true);
});

it('has proper source for refined image', () => {
let $img = $('#refined-image img');
expect($img.attr('src').startsWith('/src/')).to.equal(true);
expect($img.attr('src').startsWith('/')).to.equal(true);
});

it('has proper sources for array of images', () => {
let $img = $('#array-of-images img');
const imgsSrcs = [];
$img.each((i, img) => imgsSrcs.push(img.attribs['src']));
expect($img).to.have.a.lengthOf(2);
expect(imgsSrcs.every((img) => img.startsWith('/src/'))).to.be.true;
expect(imgsSrcs.every((img) => img.startsWith('/'))).to.be.true;
});

it('has proper attributes for optimized image through getImage', () => {
Expand All @@ -330,7 +356,7 @@ describe('astro:image', () => {

it('properly handles nested images', () => {
let $img = $('#nested-image img');
expect($img.attr('src').startsWith('/src/')).to.equal(true);
expect($img.attr('src').startsWith('/')).to.equal(true);
});
});

Expand All @@ -348,7 +374,7 @@ describe('astro:image', () => {
});

it('includes /src in the path', async () => {
expect($('img').attr('src').startsWith('/src')).to.equal(true);
expect($('img').attr('src').includes('/src')).to.equal(true);
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { APIRoute } from "../../../../../src/@types/astro";

export const get = (async ({ params, request }) => {
const url = new URL(request.url);
console.log(url)
const src = url.searchParams.get("src");

return {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
import { Image } from "astro:assets";
import myImage from "../assets/penguin1.jpg";
---

<Image src={myImage.src} alt="hello"/>
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
import { Image } from "astro:assets";
import imageOutsideProject from "../../../core-image-base/src/assets/penguin1.jpg";
---

<Image src={imageOutsideProject} alt="outside project" />

<img src={imageOutsideProject.src} />