diff options
-rw-r--r-- | packages/astro/CHANGELOG.md | 25 | ||||
-rw-r--r-- | packages/astro/src/actions/runtime/utils.ts | 7 | ||||
-rw-r--r-- | packages/astro/src/core/constants.ts | 5 | ||||
-rw-r--r-- | packages/astro/src/core/errors/dev/vite.ts | 2 | ||||
-rw-r--r-- | packages/astro/src/core/middleware/noop-middleware.ts | 6 | ||||
-rw-r--r-- | packages/astro/src/core/module-loader/vite.ts | 21 | ||||
-rw-r--r-- | packages/astro/src/vite-plugin-astro-server/route.ts | 46 | ||||
-rw-r--r-- | packages/astro/test/actions.test.js | 6 | ||||
-rw-r--r-- | packages/astro/test/fixtures/actions/src/pages/invalid.astro | 6 | ||||
-rw-r--r-- | packages/astro/test/fixtures/middleware space/src/middleware.js | 11 | ||||
-rw-r--r-- | packages/astro/test/middleware.test.js | 15 |
11 files changed, 129 insertions, 21 deletions
diff --git a/packages/astro/CHANGELOG.md b/packages/astro/CHANGELOG.md index 98332e7da..aa3d02e32 100644 --- a/packages/astro/CHANGELOG.md +++ b/packages/astro/CHANGELOG.md @@ -1126,6 +1126,31 @@ - Updated dependencies [[`83a2a64`](https://github.com/withastro/astro/commit/83a2a648418ad30f4eb781d1c1b5f2d8a8ac846e)]: - @astrojs/markdown-remark@6.0.0-alpha.0 +## 4.16.12 + +### Patch Changes + +- [#12420](https://github.com/withastro/astro/pull/12420) [`acac0af`](https://github.com/withastro/astro/commit/acac0af53466f8a381ccdac29ed2ad735d7b4e79) Thanks [@ematipico](https://github.com/ematipico)! - Fixes an issue where the dev server returns a 404 status code when a user middleware returns a valid `Response`. + +## 4.16.11 + +### Patch Changes + +- [#12305](https://github.com/withastro/astro/pull/12305) [`f5f7109`](https://github.com/withastro/astro/commit/f5f71094ec74961b4cca2ee451798abd830c617a) Thanks [@florian-lefebvre](https://github.com/florian-lefebvre)! - Fixes a case where the error overlay would not escape the message + +- [#12402](https://github.com/withastro/astro/pull/12402) [`823e73b`](https://github.com/withastro/astro/commit/823e73b164eab4115af31b1de8e978f2b4e0a95d) Thanks [@ematipico](https://github.com/ematipico)! - Fixes a case where Astro allowed to call an action without using `Astro.callAction`. This is now invalid, and Astro will show a proper error. + + ```diff + --- + import { actions } from "astro:actions"; + + -const result = actions.getUser({ userId: 123 }); + +const result = Astro.callAction(actions.getUser, { userId: 123 }); + --- + ``` + +- [#12401](https://github.com/withastro/astro/pull/12401) [`9cca108`](https://github.com/withastro/astro/commit/9cca10843912698e13d35f1bc3c493e2c96a06ee) Thanks [@bholmesdev](https://github.com/bholmesdev)! - Fixes unexpected 200 status in dev server logs for action errors and redirects. + ## 4.16.10 ### Patch Changes diff --git a/packages/astro/src/actions/runtime/utils.ts b/packages/astro/src/actions/runtime/utils.ts index 7d6a21730..35c750055 100644 --- a/packages/astro/src/actions/runtime/utils.ts +++ b/packages/astro/src/actions/runtime/utils.ts @@ -10,6 +10,8 @@ export type Locals = { _actionPayload: ActionPayload; }; +export const ACTION_API_CONTEXT_SYMBOL = Symbol.for('astro.actionAPIContext'); + export const formContentTypes = ['application/x-www-form-urlencoded', 'multipart/form-data']; export function hasContentType(contentType: string, expected: string[]) { @@ -36,3 +38,8 @@ export type MaybePromise<T> = T | Promise<T>; * `result.error.fields` will be typed with the `name` field. */ export type ErrorInferenceObject = Record<string, any>; + +export function isActionAPIContext(ctx: ActionAPIContext): boolean { + const symbol = Reflect.get(ctx, ACTION_API_CONTEXT_SYMBOL); + return symbol === true; +} diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts index 6183e1557..dc09a1f69 100644 --- a/packages/astro/src/core/constants.ts +++ b/packages/astro/src/core/constants.ts @@ -32,6 +32,11 @@ export const REWRITE_DIRECTIVE_HEADER_KEY = 'X-Astro-Rewrite'; export const REWRITE_DIRECTIVE_HEADER_VALUE = 'yes'; /** + * This header is set by the no-op Astro middleware. + */ +export const NOOP_MIDDLEWARE_HEADER = 'X-Astro-Noop'; + +/** * The name for the header used to help i18n middleware, which only needs to act on "page" and "fallback" route types. */ export const ROUTE_TYPE_HEADER = 'X-Astro-Route-Type'; diff --git a/packages/astro/src/core/errors/dev/vite.ts b/packages/astro/src/core/errors/dev/vite.ts index c944ad78d..44b41e379 100644 --- a/packages/astro/src/core/errors/dev/vite.ts +++ b/packages/astro/src/core/errors/dev/vite.ts @@ -105,6 +105,7 @@ export function enhanceViteSSRError({ } export interface AstroErrorPayload { + __isEnhancedAstroErrorPayload: true; type: ErrorPayload['type']; err: Omit<ErrorPayload['err'], 'loc'> & { name?: string; @@ -164,6 +165,7 @@ export async function getViteErrorPayload(err: ErrorWithMetadata): Promise<Astro : undefined; return { + __isEnhancedAstroErrorPayload: true, type: 'error', err: { ...err, diff --git a/packages/astro/src/core/middleware/noop-middleware.ts b/packages/astro/src/core/middleware/noop-middleware.ts index ebb7c1b34..d8ccbc2c7 100644 --- a/packages/astro/src/core/middleware/noop-middleware.ts +++ b/packages/astro/src/core/middleware/noop-middleware.ts @@ -1,3 +1,7 @@ import type { MiddlewareHandler } from '../../types/public/common.js'; +import { NOOP_MIDDLEWARE_HEADER } from '../constants.js'; -export const NOOP_MIDDLEWARE_FN: MiddlewareHandler = (_, next) => next(); +export const NOOP_MIDDLEWARE_FN: MiddlewareHandler = (ctx, next) => { + ctx.request.headers.set(NOOP_MIDDLEWARE_HEADER, 'true'); + return next(); +}; diff --git a/packages/astro/src/core/module-loader/vite.ts b/packages/astro/src/core/module-loader/vite.ts index 1b2a4423c..20aea87e2 100644 --- a/packages/astro/src/core/module-loader/vite.ts +++ b/packages/astro/src/core/module-loader/vite.ts @@ -1,6 +1,9 @@ import { EventEmitter } from 'node:events'; import path from 'node:path'; +import { pathToFileURL } from 'node:url'; import type * as vite from 'vite'; +import { collectErrorMetadata } from '../errors/dev/utils.js'; +import { getViteErrorPayload } from '../errors/dev/vite.js'; import type { ModuleLoader, ModuleLoaderEventEmitter } from './loader.js'; export function createViteLoader(viteServer: vite.ViteDevServer): ModuleLoader { @@ -43,6 +46,24 @@ export function createViteLoader(viteServer: vite.ViteDevServer): ModuleLoader { } const msg = args[0] as vite.HMRPayload; if (msg?.type === 'error') { + // If we have an error, but it didn't go through our error enhancement program, it means that it's a HMR error from + // vite itself, which goes through a different path. We need to enhance it here. + if (!(msg as any)['__isEnhancedAstroErrorPayload']) { + const err = collectErrorMetadata(msg.err, pathToFileURL(viteServer.config.root)); + getViteErrorPayload(err).then((payload) => { + events.emit('hmr-error', { + type: 'error', + err: { + message: payload.err.message, + stack: payload.err.stack, + }, + }); + + args[0] = payload; + _wsSend.apply(this, args); + }); + return; + } events.emit('hmr-error', msg); } _wsSend.apply(this, args); diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 459d24cc3..7c2a5bd9a 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -1,6 +1,7 @@ import type http from 'node:http'; import { DEFAULT_404_COMPONENT, + NOOP_MIDDLEWARE_HEADER, REROUTE_DIRECTIVE_HEADER, REWRITE_DIRECTIVE_HEADER_KEY, clientLocalsSymbol, @@ -134,7 +135,6 @@ type HandleRoute = { manifestData: ManifestData; incomingRequest: http.IncomingMessage; incomingResponse: http.ServerResponse; - status?: 404 | 500 | 200; pipeline: DevPipeline; }; @@ -142,7 +142,6 @@ export async function handleRoute({ matchedRoute, url, pathname, - status = getStatus(matchedRoute), body, origin, pipeline, @@ -197,27 +196,32 @@ export async function handleRoute({ mod = preloadedComponent; - const isDefaultPrerendered404 = - matchedRoute.route.route === '/404' && - matchedRoute.route.prerender && - matchedRoute.route.component === DEFAULT_404_COMPONENT; - renderContext = await RenderContext.create({ locals, pipeline, pathname, - middleware: isDefaultPrerendered404 ? undefined : middleware, + middleware: isDefaultPrerendered404(matchedRoute.route) ? undefined : middleware, request, routeData: route, }); let response; + let statusCode = 200; let isReroute = false; let isRewrite = false; try { response = await renderContext.render(mod); isReroute = response.headers.has(REROUTE_DIRECTIVE_HEADER); isRewrite = response.headers.has(REWRITE_DIRECTIVE_HEADER_KEY); + const statusCodedMatched = getStatusByMatchedRoute(matchedRoute); + statusCode = isRewrite + ? // Ignore `matchedRoute` status for rewrites + response.status + : // Our internal noop middleware sets a particular header. If the header isn't present, it means that the user have + // their own middleware, so we need to return what the user returns. + !response.headers.has(NOOP_MIDDLEWARE_HEADER) && !isReroute + ? response.status + : (statusCodedMatched ?? response.status); } catch (err: any) { const custom500 = getCustom500Route(manifestData); if (!custom500) { @@ -229,7 +233,7 @@ export async function handleRoute({ const preloaded500Component = await pipeline.preload(custom500, filePath500); renderContext.props.error = err; response = await renderContext.render(preloaded500Component); - status = 500; + statusCode = 500; } if (isLoggedRequest(pathname)) { @@ -239,20 +243,20 @@ export async function handleRoute({ req({ url: pathname, method: incomingRequest.method, - statusCode: isRewrite ? response.status : (status ?? response.status), + statusCode, isRewrite, reqTime: timeEnd - timeStart, }), ); } - if (response.status === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') { + + if (statusCode === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') { const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline); if (options && options.route !== fourOhFourRoute?.route) return handleRoute({ ...options, matchedRoute: fourOhFourRoute, url: new URL(pathname, url), - status: 404, body, origin, pipeline, @@ -318,18 +322,22 @@ export async function handleRoute({ // Apply the `status` override to the response object before responding. // Response.status is read-only, so a clone is required to override. - if (status && response.status !== status && (status === 404 || status === 500)) { + if (response.status !== statusCode) { response = new Response(response.body, { - status: status, + status: statusCode, headers: response.headers, }); } await writeSSRResult(request, response, incomingResponse); } -function getStatus(matchedRoute?: MatchedRoute): 404 | 500 | 200 { - if (!matchedRoute) return 404; - if (matchedRoute.route.route === '/404') return 404; - if (matchedRoute.route.route === '/500') return 500; - return 200; +/** Check for /404 and /500 custom routes to compute status code */ +function getStatusByMatchedRoute(matchedRoute?: MatchedRoute) { + if (matchedRoute?.route.route === '/404') return 404; + if (matchedRoute?.route.route === '/500') return 500; + return undefined; +} + +function isDefaultPrerendered404(route: RouteData) { + return route.route === '/404' && route.prerender && route.component === DEFAULT_404_COMPONENT; } diff --git a/packages/astro/test/actions.test.js b/packages/astro/test/actions.test.js index c34b91a7d..257bf0284 100644 --- a/packages/astro/test/actions.test.js +++ b/packages/astro/test/actions.test.js @@ -132,6 +132,12 @@ describe('Astro Actions', () => { assert.equal(data, 'Hello, ben!'); } }); + + it('Should fail when calling an action without using Astro.callAction', async () => { + const res = await fixture.fetch('/invalid/'); + const text = await res.text(); + assert.match(text, /ActionCalledFromServerError/); + }); }); describe('build', () => { diff --git a/packages/astro/test/fixtures/actions/src/pages/invalid.astro b/packages/astro/test/fixtures/actions/src/pages/invalid.astro new file mode 100644 index 000000000..908eee853 --- /dev/null +++ b/packages/astro/test/fixtures/actions/src/pages/invalid.astro @@ -0,0 +1,6 @@ +--- +import { actions } from "astro:actions"; + +// this is invalid, it should fail +const result = await actions.imageUploadInChunks(); +--- diff --git a/packages/astro/test/fixtures/middleware space/src/middleware.js b/packages/astro/test/fixtures/middleware space/src/middleware.js index ed0503c80..ac9ffba2f 100644 --- a/packages/astro/test/fixtures/middleware space/src/middleware.js +++ b/packages/astro/test/fixtures/middleware space/src/middleware.js @@ -74,4 +74,13 @@ const third = defineMiddleware(async (context, next) => { return next(); }); -export const onRequest = sequence(first, second, third); +const fourth = defineMiddleware((context, next) => { + if (context.request.url.includes('/no-route-but-200')) { + return new Response("It's OK!", { + status: 200 + }); + } + return next() +}) + +export const onRequest = sequence(first, second, third, fourth); diff --git a/packages/astro/test/middleware.test.js b/packages/astro/test/middleware.test.js index 63c2788bc..26bc06d77 100644 --- a/packages/astro/test/middleware.test.js +++ b/packages/astro/test/middleware.test.js @@ -70,6 +70,13 @@ describe('Middleware in DEV mode', () => { assert.equal($('title').html(), 'MiddlewareNoDataOrNextCalled'); }); + it('should return 200 if the middleware returns a 200 Response', async () => { + const response = await fixture.fetch('/no-route-but-200'); + assert.equal(response.status, 200); + const html = await response.text(); + assert.match(html, /It's OK!/); + }); + it('should allow setting cookies', async () => { const res = await fixture.fetch('/'); assert.equal(res.headers.get('set-cookie'), 'foo=bar'); @@ -245,6 +252,14 @@ describe('Middleware API in PROD mode, SSR', () => { assert.notEqual($('title').html(), 'MiddlewareNoDataReturned'); }); + it('should return 200 if the middleware returns a 200 Response', async () => { + const request = new Request('http://example.com/no-route-but-200'); + const response = await app.render(request); + assert.equal(response.status, 200); + const html = await response.text(); + assert.match(html, /It's OK!/); + }); + it('should correctly work for API endpoints that return a Response object', async () => { const request = new Request('http://example.com/api/endpoint'); const response = await app.render(request); |