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

fix(assets): Fix endpoint not being injected when an integration would enable experimental.assets #7829

Merged
merged 4 commits into from
Jul 27, 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/neat-balloons-doubt.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fix `astro:assets` endpoint not working in dev and SSR if `experimental.assets` was enabled by an integration (such as Starlight)
11 changes: 11 additions & 0 deletions packages/astro/src/assets/internal.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,18 @@
import type { AstroSettings } from '../@types/astro.js';
import { AstroError, AstroErrorData } from '../core/errors/index.js';
import { isLocalService, type ImageService } from './services/service.js';
import type { GetImageResult, ImageMetadata, ImageTransform } from './types.js';

export function injectImageEndpoint(settings: AstroSettings) {
settings.injectedRoutes.push({
pattern: '/_image',
entryPoint: 'astro/assets/image-endpoint',
prerender: false,
});

return settings;
}

export function isESMImportedImage(src: ImageMetadata | string): src is ImageMetadata {
return typeof src === 'object';
}
Expand Down
3 changes: 1 addition & 2 deletions packages/astro/src/cli/load-settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,9 @@ export async function loadSettings({ cmd, flags, logging }: LoadSettingsOptions)
return {} as any;
});

const mode = cmd === 'build' ? 'build' : 'dev';
if (!initialAstroConfig) return;
telemetry.record(event.eventCliSession(cmd, initialUserConfig, flags));
return createSettings(initialAstroConfig, mode, root);
return createSettings(initialAstroConfig, root);
}

export async function handleConfigError(
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/config/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function getViteConfig(inlineConfig: UserConfig) {
level: 'info',
};
const { astroConfig: config } = await openConfig({ cmd });
const settings = createSettings(config, cmd, inlineConfig.root);
const settings = createSettings(config, inlineConfig.root);
await runHookConfigSetup({ settings, command: cmd, logging });
const viteConfig = await createVite(
{
Expand Down
9 changes: 9 additions & 0 deletions packages/astro/src/core/build/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import { performance } from 'node:perf_hooks';
import type * as vite from 'vite';
import type yargs from 'yargs-parser';
import type { AstroConfig, AstroSettings, ManifestData, RuntimeMode } from '../../@types/astro';
import { injectImageEndpoint } from '../../assets/internal.js';
import {
runHookBuildDone,
runHookBuildStart,
runHookConfigDone,
runHookConfigSetup,
} from '../../integrations/index.js';
import { isServerLikeOutput } from '../../prerender/utils.js';
import { createVite } from '../create-vite.js';
import { debug, info, levels, timerMessage, warn, type LogOptions } from '../logger/core.js';
import { printHelp } from '../messages.js';
Expand Down Expand Up @@ -89,6 +91,13 @@ class AstroBuilder {
command: 'build',
logging,
});

// HACK: Since we only inject the endpoint if `experimental.assets` is on and it's possible for an integration to
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth a “Remove in 3.0” comment or would we pick it up anyway because experimental.assets will go away so this would be a type error?

Copy link
Member Author

Choose a reason for hiding this comment

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

In 3.0 it's likely that we'll need an option to disable the endpoint (ex: maybe you have your own, maybe you don't want any assets stuff), so we'll still need to check for something in the config still here. So for now I'll leave it like this and update whenever we'll add that option

// add that flag, we need to only check and inject the endpoint after running the config setup hook.
if (this.settings.config.experimental.assets && isServerLikeOutput(this.settings.config)) {
this.settings = injectImageEndpoint(this.settings);
}

this.manifest = createRouteManifest({ settings: this.settings }, this.logging);

const viteConfig = await createVite(
Expand Down
18 changes: 5 additions & 13 deletions packages/astro/src/core/config/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { fileURLToPath, pathToFileURL } from 'node:url';
import type { AstroConfig, AstroSettings, AstroUserConfig } from '../../@types/astro';
import { getContentPaths } from '../../content/index.js';
import jsxRenderer from '../../jsx/renderer.js';
import { isServerLikeOutput } from '../../prerender/utils.js';
import { markdownContentEntryType } from '../../vite-plugin-markdown/content-entry-type.js';
import { getDefaultClientDirectives } from '../client-directive/index.js';
import { AstroError, AstroErrorData } from '../errors/index.js';
Expand All @@ -14,18 +13,15 @@ import { createDefaultDevConfig } from './config.js';
import { AstroTimer } from './timer.js';
import { loadTSConfig } from './tsconfig.js';

export function createBaseSettings(config: AstroConfig, mode: 'build' | 'dev'): AstroSettings {
export function createBaseSettings(config: AstroConfig): AstroSettings {
const { contentDir } = getContentPaths(config);
return {
config,
tsConfig: undefined,
tsConfigPath: undefined,

adapter: undefined,
injectedRoutes:
config.experimental.assets && (isServerLikeOutput(config) || mode === 'dev')
? [{ pattern: '/_image', entryPoint: 'astro/assets/image-endpoint', prerender: false }]
: [],
injectedRoutes: [],
pageExtensions: ['.astro', '.html', ...SUPPORTED_MARKDOWN_FILE_EXTENSIONS],
contentEntryTypes: [markdownContentEntryType],
dataEntryTypes: [
Expand Down Expand Up @@ -108,13 +104,9 @@ export function createBaseSettings(config: AstroConfig, mode: 'build' | 'dev'):
};
}

export function createSettings(
config: AstroConfig,
mode: 'build' | 'dev',
cwd?: string
): AstroSettings {
export function createSettings(config: AstroConfig, cwd?: string): AstroSettings {
const tsconfig = loadTSConfig(cwd);
const settings = createBaseSettings(config, mode);
const settings = createBaseSettings(config);

const watchFiles = tsconfig?.exists ? [tsconfig.path, ...tsconfig.extendedPaths] : [];

Expand All @@ -136,5 +128,5 @@ export async function createDefaultDevSettings(
root = fileURLToPath(root);
}
const config = await createDefaultDevConfig(userConfig, root);
return createBaseSettings(config, 'dev');
return createBaseSettings(config);
}
8 changes: 8 additions & 0 deletions packages/astro/src/core/dev/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import type { AstroSettings, AstroUserConfig } from '../../@types/astro';

import nodeFs from 'node:fs';
import * as vite from 'vite';
import { injectImageEndpoint } from '../../assets/internal.js';
import {
runHookConfigDone,
runHookConfigSetup,
Expand Down Expand Up @@ -64,6 +65,13 @@ export async function createContainer(params: CreateContainerParams = {}): Promi
logging,
isRestart,
});

// HACK: Since we only inject the endpoint if `experimental.assets` is on and it's possible for an integration to
// add that flag, we need to only check and inject the endpoint after running the config setup hook.
if (settings.config.experimental.assets) {
settings = injectImageEndpoint(settings);
}

const { host, headers, open } = settings.config.server;

// The client entrypoint for renderers. Since these are imported dynamically
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/dev/restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export async function restartContainer({
});
info(logging, 'astro', logMsg + '\n');
let astroConfig = newConfig.astroConfig;
const settings = createSettings(astroConfig, 'dev', resolvedRoot);
const settings = createSettings(astroConfig, resolvedRoot);
await close();
return {
container: await createRestartedContainer(container, settings, needsStart),
Expand Down
4 changes: 2 additions & 2 deletions packages/astro/test/test-utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ export async function loadFixture(inlineConfig) {
* the `AstroSettings`. This function helps to create a fresh settings object that is used by the
* command functions below to prevent tests from polluting each other.
*/
const getSettings = async (mode) => {
let settings = createSettings(config, mode, fileURLToPath(cwd));
const getSettings = async () => {
let settings = createSettings(config, fileURLToPath(cwd));
if (config.integrations.find((integration) => integration.name === '@astrojs/mdx')) {
// Enable default JSX integration. It needs to come first, so unshift rather than push!
const { default: jsxRenderer } = await import('astro/jsx/renderer.js');
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/test/units/config/format.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describe('Astro config formats', () => {
logging: defaultLogging,
fsMod: fs,
});
const settings = createSettings(astroConfig, 'dev');
const settings = createSettings(astroConfig);

await runInContainer({ fs, root, settings }, () => {
expect(true).to.equal(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const logging = defaultLogging;

async function sync({ fs, config = {} }) {
const astroConfig = await validateConfig(config, fileURLToPath(root), 'prod');
const settings = createSettings(astroConfig, 'build', fileURLToPath(root));
const settings = createSettings(astroConfig, fileURLToPath(root));

return _sync(settings, { logging, fs });
}
Expand Down
6 changes: 3 additions & 3 deletions packages/astro/test/units/dev/restart.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ describe('dev container restarts', () => {
cmd: 'dev',
logging: defaultLogging,
});
const settings = createSettings(astroConfig, 'dev');
const settings = createSettings(astroConfig);

let restart = await createContainerWithAutomaticRestart({
params: { fs, root, settings },
Expand Down Expand Up @@ -167,7 +167,7 @@ describe('dev container restarts', () => {
cmd: 'dev',
logging: defaultLogging,
});
const settings = createSettings(astroConfig, 'dev', fileURLToPath(root));
const settings = createSettings(astroConfig, fileURLToPath(root));

let restart = await createContainerWithAutomaticRestart({
params: { fs, root, settings },
Expand Down Expand Up @@ -199,7 +199,7 @@ describe('dev container restarts', () => {
cmd: 'dev',
logging: defaultLogging,
});
const settings = createSettings(astroConfig, 'dev', fileURLToPath(root));
const settings = createSettings(astroConfig, fileURLToPath(root));

let restart = await createContainerWithAutomaticRestart({
params: { fs, root, settings },
Expand Down