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(Astro global): parity with ctx.site #10325

Merged
merged 9 commits into from
Mar 7, 2024
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/sour-apples-try.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Fixes an issue where `ctx.site` included the configured `base` in API routes and middleware, unlike `Astro.site` in astro pages.
2 changes: 1 addition & 1 deletion packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2701,7 +2701,7 @@ export interface SSRResult {
slots: Record<string, any> | null
): AstroGlobal;
resolve: (s: string) => Promise<string>;
response: ResponseInit;
response: AstroGlobal["response"];
renderers: SSRLoadedRenderer[];
/**
* Map of directive name (e.g. `load`) to the directive script code
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/base-pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export abstract class Pipeline {
/**
* Used for `Astro.site`.
*/
readonly site = manifest.site
readonly site = manifest.site ? new URL(manifest.site) : undefined,
) {
this.internalMiddleware = [
createI18nMiddleware(i18n, manifest.base, manifest.trailingSlash, manifest.buildFormat),
Expand Down
4 changes: 1 addition & 3 deletions packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -609,9 +609,7 @@ function createBuildManifest(
renderers,
base: settings.config.base,
assetsPrefix: settings.config.build.assetsPrefix,
site: settings.config.site
? new URL(settings.config.base, settings.config.site).toString()
: settings.config.site,
site: settings.config.site,
componentMetadata: internals.componentMetadata,
i18n: i18nManifest,
buildFormat: settings.config.build.format,
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/core/compile/compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ export async function compile({
normalizedFilename: normalizeFilename(filename, astroConfig.root),
sourcemap: 'both',
internalURL: 'astro/compiler-runtime',
// TODO: this is no longer neccessary for `Astro.site`
// but it somehow allows working around caching issues in content collections for some tests
astroGlobalArgs: JSON.stringify(astroConfig.site),
scopedStyleStrategy: astroConfig.scopedStyleStrategy,
resultScopedSlot: true,
Expand Down
15 changes: 14 additions & 1 deletion packages/astro/src/core/errors/errors-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -792,7 +792,7 @@ export const MiddlewareNotAResponse = {
* @docs
* @description
*
* Thrown in development mode when `locals` is overwritten with something that is not an object
* Thrown when `locals` is overwritten with something that is not an object
*
* For example:
* ```ts
Expand All @@ -811,6 +811,19 @@ export const LocalsNotAnObject = {
hint: 'If you tried to remove some information from the `locals` object, try to use `delete` or set the property to `undefined`.',
} satisfies ErrorData;

/**
* @docs
* @description
* Thrown when a value is being set as the `headers` field on the `ResponseInit` object available as `Astro.response`.
*/
export const AstroResponseHeadersReassigned = {
name: 'AstroResponseHeadersReassigned',
title: '`Astro.response.headers` must not be reassigned.',
message:
'Individual headers can be added to and removed from `Astro.response.headers`, but it must not be replaced with another instance of `Headers` altogether.',
hint: 'Consider using `Astro.response.headers.add()`, and `Astro.response.headers.delete()`.',
} satisfies ErrorData;

/**
* @docs
* @description
Expand Down
2 changes: 0 additions & 2 deletions packages/astro/src/core/middleware/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ function createContext({
return (currentLocale ??= computeCurrentLocale(
route,
userDefinedLocales,
undefined,
undefined
));
},
url,
Expand Down
194 changes: 135 additions & 59 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import type {
APIContext,
AstroGlobal,
AstroGlobalPartial,
ComponentInstance,
MiddlewareHandler,
RouteData,
SSRResult,
} from '../@types/astro.js';
import {
computeCurrentLocale,
Expand All @@ -11,21 +14,20 @@ import {
} from '../i18n/utils.js';
import { renderEndpoint } from '../runtime/server/endpoint.js';
import { renderPage } from '../runtime/server/index.js';
import { renderRedirect } from './redirects/render.js';
import {
ASTRO_VERSION,
REROUTE_DIRECTIVE_HEADER,
ROUTE_TYPE_HEADER,
clientAddressSymbol,
clientLocalsSymbol,
responseSentSymbol,
} from './constants.js';
import { attachCookiesToResponse } from './cookies/index.js';
import { AstroCookies } from './cookies/index.js';
import { attachCookiesToResponse, AstroCookies } from './cookies/index.js';
import { AstroError, AstroErrorData } from './errors/index.js';
import { callMiddleware } from './middleware/callMiddleware.js';
import { sequence } from './middleware/index.js';
import { renderRedirect } from './redirects/render.js';
import { createResult } from './render/index.js';
import { type Pipeline, getParams, getProps } from './render/index.js';
import { type Pipeline, getParams, getProps, Slots } from './render/index.js';

export class RenderContext {
private constructor(
Expand Down Expand Up @@ -135,38 +137,15 @@ export class RenderContext {
const generator = `Astro v${ASTRO_VERSION}`;
const redirect = (path: string, status = 302) =>
new Response(null, { status, headers: { Location: path } });
const site = pipeline.site ? new URL(pipeline.site) : undefined;
return {
cookies,
get clientAddress() {
return renderContext.clientAddress()
},
get currentLocale() {
return renderContext.computeCurrentLocale();
},
generator,
params,
get preferredLocale() {
return renderContext.computePreferredLocale();
},
get preferredLocaleList() {
return renderContext.computePreferredLocaleList();
},
props,
redirect,
request,
site,
url,
get clientAddress() {
if (clientAddressSymbol in request) {
return Reflect.get(request, clientAddressSymbol) as string;
}
if (pipeline.adapterName) {
throw new AstroError({
...AstroErrorData.ClientAddressNotAvailable,
message: AstroErrorData.ClientAddressNotAvailable.message(pipeline.adapterName),
});
} else {
throw new AstroError(AstroErrorData.StaticClientAddressNotAvailable);
}
},
get locals() {
return renderContext.locals;
},
Expand All @@ -181,53 +160,142 @@ export class RenderContext {
Reflect.set(request, clientLocalsSymbol, val);
}
},
params,
get preferredLocale() {
return renderContext.computePreferredLocale();
},
get preferredLocaleList() {
return renderContext.computePreferredLocaleList();
},
props,
redirect,
request,
site: pipeline.site,
url,
};
}

async createResult(mod: ComponentInstance) {
const { cookies, locals, params, pathname, pipeline, request, routeData, status } = this;
const { cookies, pathname, pipeline, routeData, status } = this;
const {
adapterName,
clientDirectives,
compressHTML,
i18n,
manifest,
logger,
renderers,
resolve,
site,
serverLike,
resolve
} = pipeline;
const { links, scripts, styles } = await pipeline.headElements(routeData);
const componentMetadata =
(await pipeline.componentMetadata(routeData)) ?? manifest.componentMetadata;
const { defaultLocale, locales, strategy } = i18n ?? {};
const headers = new Headers({ 'Content-Type': 'text/html' });
const partial = Boolean(mod.partial);
return createResult({
adapterName,
const response = {
status,
statusText: 'OK',
get headers() {
return headers
},
// Disallow `Astro.response.headers = new Headers`
set headers(_) {
throw new AstroError(AstroErrorData.AstroResponseHeadersReassigned);
}
} satisfies AstroGlobal["response"];

// Create the result object that will be passed into the renderPage function.
// This object starts here as an empty shell (not yet the result) but then
// calling the render() function will populate the object with scripts, styles, etc.
const result: SSRResult = {
clientDirectives,
componentMetadata,
compressHTML,
cookies,
defaultLocale,
locales,
locals,
logger,
/** This function returns the `Astro` faux-global */
createAstro: (astroGlobal, props, slots) => this.createAstro(result, astroGlobal, props, slots),
links,
params,
partial,
pathname,
renderers,
resolve,
request,
route: routeData.route,
strategy,
site,
response,
scripts,
ssr: serverLike,
status,
styles,
});
_metadata: {
hasHydrationScript: false,
rendererSpecificHydrationScripts: new Set(),
hasRenderedHead: false,
hasDirectives: new Set(),
headInTree: false,
extraHead: [],
propagators: new Set(),
},
};

return result;
}

createAstro(
result: SSRResult,
astroGlobalPartial: AstroGlobalPartial,
props: Record<string, any>,
slotValues: Record<string, any> | null
): AstroGlobal {
const renderContext = this;
const { cookies, locals, params, pipeline, request, url } = this;
const { response } = result;
const redirect = (path: string, status = 302) => {
// If the response is already sent, error as we cannot proceed with the redirect.
if ((request as any)[responseSentSymbol]) {
throw new AstroError({
...AstroErrorData.ResponseSentError,
});
}
return new Response(null, { status, headers: { Location: path } });
}
const slots = new Slots(result, slotValues, pipeline.logger) as unknown as AstroGlobal['slots'];

// `Astro.self` is added by the compiler
const astroGlobalCombined: Omit<AstroGlobal, 'self'> = {
...astroGlobalPartial,
cookies,
get clientAddress() {
return renderContext.clientAddress()
},
get currentLocale() {
return renderContext.computeCurrentLocale();
},
params,
get preferredLocale() {
return renderContext.computePreferredLocale();
},
get preferredLocaleList() {
return renderContext.computePreferredLocaleList();
},
props,
locals,
redirect,
request,
response,
slots,
site: pipeline.site,
url,
};

return astroGlobalCombined as AstroGlobal
}

clientAddress() {
const { pipeline, request } = this;
if (clientAddressSymbol in request) {
return Reflect.get(request, clientAddressSymbol) as string;
}
if (pipeline.adapterName) {
throw new AstroError({
...AstroErrorData.ClientAddressNotAvailable,
message: AstroErrorData.ClientAddressNotAvailable.message(pipeline.adapterName),
});
} else {
throw new AstroError(AstroErrorData.StaticClientAddressNotAvailable);
}
}

/**
Expand All @@ -242,13 +310,21 @@ export class RenderContext {
routeData,
} = this;
if (!i18n) return;

const { defaultLocale, locales, strategy } = i18n;
return (this.#currentLocale ??= computeCurrentLocale(
routeData.route,
locales,
strategy,
defaultLocale
));

const fallbackTo = (
strategy === 'pathname-prefix-other-locales' ||
strategy === 'domains-prefix-other-locales'
) ? defaultLocale : undefined

// TODO: look into why computeCurrentLocale() needs routeData.route to pass ctx.currentLocale tests,
// and url.pathname to pass Astro.currentLocale tests.
// A single call with `routeData.pathname ?? routeData.route` as the pathname still fails.
Comment on lines +321 to +323
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this puzzled me, too. It requires some investigation. We definitely know that url.pathname doesn't make the cut because during the build, it contains the actual name of the file, e.g. /index.html//blog/en.html. However, in dev it contains the correct data.

return (this.#currentLocale ??=
computeCurrentLocale(routeData.route, locales) ??
computeCurrentLocale(url.pathname, locales) ??
fallbackTo);
}

#preferredLocale: APIContext['preferredLocale'];
Expand Down
2 changes: 1 addition & 1 deletion packages/astro/src/core/render/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { Pipeline } from '../base-pipeline.js';
export { Pipeline } from '../base-pipeline.js';
export { getParams, getProps } from './params-and-props.js';
export { loadRenderer } from './renderer.js';
export { createResult } from './result.js';
export { Slots } from './result.js';

export interface SSROptions {
/** The pipeline instance */
Expand Down
Loading
Loading