fix: Adjust fail method and ActionFailure type (#11260)

* fix: Adjust fail method and ActionFailure type - add ActionFailure interface and use that publicly instead of the class. Prevents the false impression that you could do "instanceof" on the return type, fixes #10361 - add uniqueSymbol to ActionFailure instance so we can distinguish it from a regular return with the same shape - fix setup to actually run test/types * test * regenerate types --------- Co-authored-by: Rich Harris <[email protected]>
sveltejs · Dec 11, 2023 · c59375c · c59375c
1 parent 608e3ef
commit c59375c
Show file tree

Hide file tree

Showing 8 changed files with 84 additions and 43 deletions.
diff --git a/.changeset/swift-clocks-kneel.md b/.changeset/swift-clocks-kneel.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': patch
+---
+
+fix: Adjust fail method and ActionFailure type
diff --git a/packages/kit/package.json b/packages/kit/package.json
@@ -57,7 +57,7 @@
 	],
 	"scripts": {
 		"lint": "prettier --config ../../.prettierrc --check .",
-		"check": "tsc",
+		"check": "tsc && cd ./test/types && tsc",
 		"check:all": "tsc && pnpm -r --filter=\"./**\" check",
 		"format": "prettier --config ../../.prettierrc --write .",
 		"test": "pnpm test:unit && pnpm test:integration",

diff --git a/packages/kit/src/exports/index.js b/packages/kit/src/exports/index.js
@@ -156,14 +156,31 @@ export function text(body, init) {
 	});
 }
 
+/**
+ * Create an `ActionFailure` object.
+ * @param {number} status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599.
+ * @overload
+ * @param {number} status
+ * @returns {import('./public.js').ActionFailure<undefined>}
+ */
 /**
  * Create an `ActionFailure` object.
  * @template {Record<string, unknown> | undefined} [T=undefined]
  * @param {number} status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599.
- * @param {T} [data] Data associated with the failure (e.g. validation errors)
- * @returns {ActionFailure<T>}
+ * @param {T} data Data associated with the failure (e.g. validation errors)
+ * @overload
+ * @param {number} status
+ * @param {T} data
+ * @returns {import('./public.js').ActionFailure<T>}
+ */
+/**
+ * Create an `ActionFailure` object.
+ * @param {number} status
+ * @param {any} [data]
+ * @returns {import('./public.js').ActionFailure<any>}
  */
 export function fail(status, data) {
+	// @ts-expect-error unique symbol missing
 	return new ActionFailure(status, data);
 }
 

diff --git a/packages/kit/src/exports/public.d.ts b/packages/kit/src/exports/public.d.ts
@@ -17,12 +17,10 @@ import {
 	RequestOptions,
 	RouteSegment
 } from '../types/private.js';
-import { ActionFailure } from '../runtime/control.js';
 import { BuildData, SSRNodeLoader, SSRRoute, ValidatedConfig } from 'types';
 import type { PluginOptions } from '@sveltejs/vite-plugin-svelte';
 
 export { PrerenderOption } from '../types/private.js';
-export { ActionFailure };
 
 /**
  * [Adapters](https://kit.svelte.dev/docs/adapters) are responsible for taking the production build and turning it into something that can be deployed to a platform of your choosing.
@@ -58,6 +56,14 @@ type OptionalUnion<
 	A extends keyof U = U extends U ? keyof U : never
 > = U extends unknown ? { [P in Exclude<A, keyof U>]?: never } & U : never;
 
+declare const uniqueSymbol: unique symbol;
+
+export interface ActionFailure<T extends Record<string, unknown> | undefined = undefined> {
+	status: number;
+	data: T;
+	[uniqueSymbol]: true; // necessary or else UnpackValidationError could wrongly unpack objects with the same shape as ActionFailure
+}
+
 type UnpackValidationError<T> = T extends ActionFailure<infer X>
 	? X
 	: T extends void

diff --git a/packages/kit/src/runtime/control.js b/packages/kit/src/runtime/control.js
@@ -36,7 +36,7 @@ export class Redirect {
 export class ActionFailure {
 	/**
 	 * @param {number} status
-	 * @param {T} [data]
+	 * @param {T} data
 	 */
 	constructor(status, data) {
 		this.status = status;

diff --git a/packages/kit/test/types/actions.test.ts b/packages/kit/test/types/actions.test.ts
@@ -1,9 +1,10 @@
-import Kit from '@sveltejs/kit';
+import * as Kit from '@sveltejs/kit';
 
 // Test: Action types inferred correctly and transformed into a union
 type Actions = {
 	foo: () => Promise<void>;
 	bar: () => Promise<{ success: boolean } | Kit.ActionFailure<{ message: string }>>;
+	baz: () => Kit.ActionFailure<{ foo: string }> | { status: number; data: string };
 };
 
 let form: Kit.AwaitedActions<Actions> = null as any;
@@ -23,3 +24,17 @@ form2.message = '';
 form2.success = true;
 // @ts-expect-error - cannot both be present at the same time
 form2 = { message: '', success: true };
+
+// Test: ActionFailure is correctly infered to be different from the normal return type even if they have the same shape
+type Actions3 = {
+	bar: () => Kit.ActionFailure<{ foo: string }> | { status: number; data: { bar: string } };
+};
+let form3: Kit.AwaitedActions<Actions3> = null as any;
+form3.foo = '';
+form3.status = 200;
+// @ts-expect-error - cannot both be present at the same time
+form3 = { foo: '', status: 200 };
+
+let foo: any = null;
+// @ts-expect-error ActionFailure is not a class and so you can't do instanceof
+foo instanceof Kit.ActionFailure;
diff --git a/packages/kit/test/types/load.test.ts b/packages/kit/test/types/load.test.ts
@@ -1,20 +1,12 @@
-import Kit, { Deferred } from '@sveltejs/kit';
+import * as Kit from '@sveltejs/kit';
 
 // Test: Return types inferred correctly and transformed into a union
-type LoadReturn1 = { success: true } | { message: Promise<string> };
+type LoadReturn1 =
+	| { success: true; message?: undefined }
+	| { success?: undefined; message: string };
 
-let result1: Kit.AwaitedProperties<LoadReturn1> = null as any;
+let result1: Kit.LoadProperties<LoadReturn1> = null as any;
 result1.message = '';
 result1.success = true;
 // @ts-expect-error - cannot both be present at the same time
 result1 = { message: '', success: true };
-
-// Test: Return types keep promise for Deferred
-type LoadReturn2 = { success: true } | Deferred<{ message: Promise<string>; eager: true }>;
-
-let result2: Kit.AwaitedProperties<LoadReturn2> = null as any;
-result2.message = Promise.resolve('');
-result2.eager = true;
-result2.success = true;
-// @ts-expect-error - cannot both be present at the same time
-result2 = { message: '', success: true };
diff --git a/packages/kit/types/index.d.ts b/packages/kit/types/index.d.ts
@@ -38,6 +38,14 @@ declare module '@sveltejs/kit' {
 		A extends keyof U = U extends U ? keyof U : never
 	> = U extends unknown ? { [P in Exclude<A, keyof U>]?: never } & U : never;
 
+	const uniqueSymbol: unique symbol;
+
+	export interface ActionFailure<T extends Record<string, unknown> | undefined = undefined> {
+		status: number;
+		data: T;
+		[uniqueSymbol]: true; // necessary or else UnpackValidationError could wrongly unpack objects with the same shape as ActionFailure
+	}
+
 	type UnpackValidationError<T> = T extends ActionFailure<infer X>
 		? X
 		: T extends void
@@ -1494,28 +1502,6 @@ declare module '@sveltejs/kit' {
 	}
 
 	type TrailingSlash = 'never' | 'always' | 'ignore';
-	class HttpError_1 {
-
-		constructor(status: number, body: {
-			message: string;
-		} extends App.Error ? (App.Error | string | undefined) : App.Error);
-		status: number;
-		body: App.Error;
-		toString(): string;
-	}
-	class Redirect_1 {
-
-		constructor(status: 300 | 301 | 302 | 303 | 304 | 305 | 306 | 307 | 308, location: string);
-		status: 300 | 301 | 302 | 303 | 304 | 305 | 306 | 307 | 308;
-		location: string;
-	}
-
-	export class ActionFailure<T extends Record<string, unknown> | undefined = undefined> {
-
-		constructor(status: number, data?: T | undefined);
-		status: number;
-		data: T | undefined;
-	}
 	interface Asset {
 		file: string;
 		size: number;
@@ -1739,12 +1725,17 @@ declare module '@sveltejs/kit' {
 	 * @param init Options such as `status` and `headers` that will be added to the response. A `Content-Length` header will be added automatically.
 	 */
 	export function text(body: string, init?: ResponseInit | undefined): Response;
+	/**
+	 * Create an `ActionFailure` object.
+	 * @param status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599.
+	 * */
+	export function fail(status: number): ActionFailure<undefined>;
 	/**
 	 * Create an `ActionFailure` object.
 	 * @param status The [HTTP status code](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status#client_error_responses). Must be in the range 400-599.
 	 * @param data Data associated with the failure (e.g. validation errors)
 	 * */
-	export function fail<T extends Record<string, unknown> | undefined = undefined>(status: number, data?: T | undefined): ActionFailure<T>;
+	export function fail<T extends Record<string, unknown> | undefined = undefined>(status: number, data: T): ActionFailure<T>;
 	/**
 	 * Populate a route ID with params to resolve a pathname.
 	 * @example
@@ -1762,6 +1753,21 @@ declare module '@sveltejs/kit' {
 	export type LessThan<TNumber extends number, TArray extends any[] = []> = TNumber extends TArray['length'] ? TArray[number] : LessThan<TNumber, [...TArray, TArray['length']]>;
 	export type NumericRange<TStart extends number, TEnd extends number> = Exclude<TEnd | LessThan<TEnd>, LessThan<TStart>>;
 	export const VERSION: string;
+	class HttpError_1 {
+
+		constructor(status: number, body: {
+			message: string;
+		} extends App.Error ? (App.Error | string | undefined) : App.Error);
+		status: number;
+		body: App.Error;
+		toString(): string;
+	}
+	class Redirect_1 {
+
+		constructor(status: 300 | 301 | 302 | 303 | 304 | 305 | 306 | 307 | 308, location: string);
+		status: 301 | 302 | 303 | 307 | 308 | 300 | 304 | 305 | 306;
+		location: string;
+	}
 }
 
 declare module '@sveltejs/kit/hooks' {