[fix] #6823 - some properties in type of object returned from ActionData is missing #6869

williamviktorsson · 2022-09-17T22:26:50Z

Fixes #6823
Fixes #6822
Also fixes the example from the docs: https://kit.svelte.dev/docs/form-actions#anatomy-of-an-action-validation-errors
Also makes sure types generated through the use of the invalid(status,data) helper function behave similarly to types generated by simply returning objects in load functions / actions.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

changeset-bot · 2022-09-17T22:26:53Z

🦋 Changeset detected

Latest commit: 22dd00f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

williamviktorsson · 2022-09-17T22:47:47Z

Seems like checks are failing cause vite 3.1.2 is out?

repsac-by · 2022-09-18T05:55:18Z

Fixes #6832

Fixes #6823

williamviktorsson · 2022-09-18T06:11:28Z

Fixes #6832

Fixes #6823

Thank you! Fixed.

david-plugge · 2022-09-18T14:34:50Z

@williamviktorsson I´ve found a simpler (more readable) way:

export type AwaitedActions<T extends Record<string, (...args: any) => any>> = {
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T];

type OptionalUnion<U, A = Partial<Record<KeysOfUnion<U>, never>>> = U extends unknown
	? Omit<A, keyof U> & U
	: never;

type KeysOfUnion<T> = T extends T ? keyof T : never;

EDIT: forget about this, this produces incorrect types

I don´t like the way the type is displayed in vscode. Not sure if there is a way to make that more readable.

williamviktorsson · 2022-09-18T14:47:00Z

@williamviktorsson I´ve found a simpler (more readable) way:

export type AwaitedActions<T extends Record<string, (...args: any) => any>> = {
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T];

type OptionalUnion<U, A = Partial<Record<KeysOfUnion<U>, never>>> = U extends unknown
	? Omit<A, keyof U> & U
	: never;

type KeysOfUnion<T> = T extends T ? keyof T : never;

I don´t like the way the type is displayed in vscode. Not sure if there is a way to make that more readable.

I was thinking about this today. The autocompletion is as good as it will get but its rather difficult to decipher the types from mouseover on the actiondata type.

The type unions should stay as the PR but it would be nice to exclude all the Omit, Partial, Record types from the final type and just pull out the keys and values of the inner objects.

david-plugge · 2022-09-18T14:49:11Z

This one seems a little better (and shorter):

export type AwaitedActions<T extends Record<string, (...args: any) => any>> = {
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T];

type OptionalUnion<
	U extends Record<string, unknown>,
	A extends keyof U = U extends U ? keyof U : never
> = U extends unknown ? { [P in Exclude<A, keyof U>]?: never } & U : never;

Not sure if we can combine the two objects in another way so we can get rid of the &

cdcarson · 2022-09-18T15:30:16Z

@williamviktorsson and @david-plugge Thanks for this.

Question about invalid() and how this PR would clarify/simplify the typing. Right now I have this. It works, but it doesn't make me particularly happy...

// src/routes/auth/sign-in/+page.server.ts

type Success = {
  authRedirect: string;
  message: string;
};

// arghh...
type Invalid = {
  status: number;
  data: TypedForm<SignInFormData>;
};

export const actions: Actions = {
  default: async (event: RequestEvent): Promise<Success | Invalid> => {
    try {
      // return Success for progressive fetch request, throw redirect otherwise
    } catch (error) {
      // FormValidationError is my class (not the SK ValidationError)
      if (error instanceof FormValidationError) {
        return invalid(StatusCodes.BAD_REQUEST, {
          data: error.data,
          errors: error.errors
        });
      }
      throw error;
    }
  }
};

After this PR, could I get rid of my Invalid type and just do this?

 default: async (event: RequestEvent): Promise<Success | TypedForm<SignInFormData>> => {...}

david-plugge · 2022-09-18T15:33:02Z

Here is another solution. I´m not able to mark the undefined properties as optional yet... Still playing around a bit.

export type AwaitedActions<T extends Record<string, (...args: any) => any>> = {
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T];

type OptionalUnion<
	U extends Record<string, unknown>,
	A = Record<U extends U ? keyof U : never, never>
> = U extends unknown ? { [P in keyof U | keyof A]: P extends keyof U ? U[P] : undefined } : never;

david-plugge · 2022-09-18T15:38:15Z

@cdcarson yes that should work!

Here is what i get with my latest implementation

import { invalid, type ValidationError, type RequestEvent } from '@sveltejs/kit'

class FormValidationError {
	data!: number;
	errors!: string;
}

export const actions = {
	default: async (event: RequestEvent): Promise<{ success: true} | ValidationError<FormValidationError>> => {
		try {
			// return Success for progressive fetch request, throw redirect otherwise
			return {
				success: true
			};
		} catch (error) {
			// FormValidationError is my class (not the SK ValidationError)
			if (error instanceof FormValidationError) {
				return invalid(StatusCodes.BAD_REQUEST, {
					data: error.data,
					errors: error.errors
				});
			}
			throw error;
		}
	}
};

Edit:

To make this actually work the ValidationError interface must be exported

cdcarson · 2022-09-18T15:48:30Z

To make this actually work the ValidationError interface must be exported

@david-plugge Cool. You are talking about the ValidationError from @sveltejs/kit, right?

williamviktorsson · 2022-09-18T15:52:41Z

@cdcarson i saw you had a related issue up, and there’s another one related to the exact thing David is helping us solve.

williamviktorsson · 2022-09-18T15:57:24Z

@cdcarson yes that should work!

Here is what i get with my latest implementation

import { invalid, type ValidationError, type RequestEvent } from '@sveltejs/kit'

class FormValidationError {
	data!: number;
	errors!: string;
}

export const actions = {
	default: async (event: RequestEvent): Promise<{ success: true} | ValidationError<FormValidationError>> => {
		try {
			// return Success for progressive fetch request, throw redirect otherwise
			return {
				success: true
			};
		} catch (error) {
			// FormValidationError is my class (not the SK ValidationError)
			if (error instanceof FormValidationError) {
				return invalid(StatusCodes.BAD_REQUEST, {
					data: error.data,
					errors: error.errors
				});
			}
			throw error;
		}
	}
};

Edit:

To make this actually work the ValidationError interface must be exported

Are you able to make undefined types data?: never rather than data: undefined

david-plugge · 2022-09-18T16:01:09Z

Are you able to make undefined types data?: never rather than data: undefined

I was trying exactly that for like half an hour, but i can´t figure it out. If that behaviour is preferred this version should be used.

david-plugge · 2022-09-18T16:14:02Z

As a reminder, the ValidationError interface does need a unique symbol. Otherwise a returned object { status: number, data: Record<string, unknown> } is also unpacked which is not the correct behaviour.

With the current sveltekit version:

export const actions: Actions = {
	async signin({ request }) {
		if (fail) {
			return {
				status: 400,
				data: {
					fail: true
				}
			};
		}

		return {
			success: true
		};
	}
};

With a special hidden property added to the ValidationError interface:

interface ValidationError<T extends Record<string, unknown> | undefined = undefined> {
	__sveltekit_validation_error__: unknown
	status: number;
	data: T;
}

dummdidumm · 2022-09-18T17:06:16Z

Interesting, sounds very related to #6822 aswell. I hear you about the ValidationError thing, one of the limitations of structural typing. Will look into this in more detail tomorrow.

cdcarson · 2022-09-18T17:15:06Z

Oh, I see what you're doing. Ignore my last comment.

dummdidumm · 2022-09-18T17:24:50Z

If anyone wants to keep experimenting in the meantime, this type helps unpacking the resulting type which helps make that type more readable:

type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;

Playground link

david-plugge · 2022-09-18T19:41:53Z

If anyone wants to keep experimenting in the meantime, this type helps unpacking the resulting type which helps make that type more readable:

That`s clean! Didn´t knew that is even possible, all my solutions just god rid of the optional modifier.

So something like this should be the final solution (didn´t test it):

export type AwaitedActions<T extends Record<string, (...args: any) => any>> = Expand<{
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T]>;

type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;
type OptionalUnion<
	U extends Record<string, unknown>,
	A extends keyof U = U extends U ? keyof U : never
> = U extends unknown ? { [P in Exclude<A, keyof U>]?: never } & U : never;

david-plugge · 2022-09-18T20:02:39Z

Too keep it consistent the same should be aplied to load functions since they behave the same when the data is not returned directly:

export const load = () => {
	if (userLoggedIn) {
		const payload = {
			posts: allPosts,
			somePrivateStuff
		}
		return payload;
	}
	const payload = {
		posts: allPosts
	}
	return payload;
}

williamviktorsson · 2022-09-18T20:23:27Z

If anyone wants to keep experimenting in the meantime, this type helps unpacking the resulting type which helps make that type more readable:

That`s clean! Didn´t knew that is even possible, all my solutions just god rid of the optional modifier.

So something like this should be the final solution (didn´t test it):
export type AwaitedActions<T extends Record<string, (...args: any) => any>> = Expand<{
	[Key in keyof T]: OptionalUnion<UnpackValidationError<Awaited<ReturnType<T[Key]>>>>;
}[keyof T]>;

type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;
type OptionalUnion<
	U extends Record<string, unknown>,
	A extends keyof U = U extends U ? keyof U : never
> = U extends unknown ? { [P in Exclude<A, keyof U>]?: never } & U : never;

tried it, it's pretty much spot on. Updating PR.

williamviktorsson · 2022-09-18T20:46:19Z

Too keep it consistent the same should be aplied to load functions since they behave the same when the data is not returned directly:
export const load = () => {
	if (userLoggedIn) {
		const payload = {
			posts: allPosts,
			somePrivateStuff
		}
		return payload;
	}
	const payload = {
		posts: allPosts
	}
	return payload;
}

I gave you an invite to the PR fork if you want to have a go at this.

david-plugge · 2022-09-19T06:52:40Z

Too keep it consistent the same should be aplied to load functions since they behave the same when the data is not returned directly:
export const load = () => {
	if (userLoggedIn) {
		const payload = {
			posts: allPosts,
			somePrivateStuff
		}
		return payload;
	}
	const payload = {
		posts: allPosts
	}
	return payload;
}

@dummdidumm what do you think about this?

Edit:

Maybe Expand should be used on every Data type (LayoutData, LayoutServerData, ...) so it´s always easy to read.

// .svelte-kit/types/src/routes/$types.d.ts

import type * as Kit from '@sveltejs/kit';

// Makes sure a type is "repackaged" and therefore more readable
type Expand<T> = T extends infer O ? { [K in keyof O]: O[K] } : never;
type RouteParams = {  }
type MaybeWithVoid<T> = {} extends T ? T | void : T;
export type RequiredKeys<T> = { [K in keyof T]-?: {} extends { [P in K]: T[K] } ? never : K; }[keyof T];
type OutputDataShape<T> = MaybeWithVoid<Omit<App.PageData, RequiredKeys<T>> & Partial<Pick<App.PageData, keyof T & keyof App.PageData>> & Record<string, any>>
type EnsureParentData<T> = T extends null | undefined ? {} : T;
type PageServerParentData = EnsureParentData<LayoutServerData>;
type PageParentData = EnsureParentData<LayoutData>;
type LayoutParams = RouteParams & {  }
type LayoutParentData = EnsureParentData<{}>;

export type PageServerLoad<OutputData extends OutputDataShape<PageServerParentData> = OutputDataShape<PageServerParentData>> = Kit.ServerLoad<RouteParams, PageServerParentData, OutputData>;
export type PageServerLoadEvent = Parameters<PageServerLoad>[0];
export type ActionData = Expand<Kit.AwaitedActions<typeof import('./proxy+page.server.js').actions>>;
export type PageServerData = Expand<Kit.AwaitedProperties<Awaited<ReturnType<typeof import('./proxy+page.server.js').load>>><;
export type PageData = Expand<Omit<PageParentData, keyof PageServerData> & PageServerData>;
export type Action = Kit.Action<RouteParams>
export type Actions = Kit.Actions<RouteParams>
export type LayoutServerData = Expand<null>;
export type LayoutData = Expand<LayoutParentData>;
export type RequestEvent = Kit.RequestEvent<RouteParams>;

If you only use the server load PageData is not readable at all

export type PageServerData = Kit.AwaitedProperties<Awaited<ReturnType<typeof import('./proxy+page.server.js').load>>>;
export type PageData = Omit<PageParentData, keyof PageServerData> & PageServerData;

dummdidumm · 2022-09-19T07:41:25Z

Both good points! Do you want to create a separate PR for that? I'll merge this one to keep it scoped to the form action typings.

williamviktorsson · 2022-09-19T09:23:44Z

Great job @david-plugge , you deserve all the credit. I think this closed three issues.

cdcarson · 2022-09-19T17:55:49Z

Hey @williamviktorsson , @david-plugge and @dummdidumm -- this is really, really great. Works perfectly, even without explicit types, even when I wrap the code to allow me to follow my throw/return preferences. Thanks!

[fix] sveltejs#6823

aab93d3

vercel bot deployed to Preview – kit September 17, 2022 22:27 View deployment

williamviktorsson and others added 2 commits September 18, 2022 00:54

.changeset

a7a76a7

Merge branch 'master' into HEAD

66a2949

vercel bot deployed to Preview – kit September 17, 2022 23:01 View deployment

This comment was marked as off-topic.

Sign in to view

vercel bot deployed to Preview – kit September 18, 2022 20:35 View deployment

vercel bot deployed to Preview – kit September 18, 2022 20:44 View deployment

removed pubspec upg

12b2936

williamviktorsson force-pushed the patch-2 branch from 20767ea to 12b2936 Compare September 18, 2022 20:54

vercel bot deployed to Preview – kit September 18, 2022 20:55 View deployment

comments

f73653e

vercel bot deployed to Preview – kit September 19, 2022 06:48 View deployment

fix

22dd00f

vercel bot deployed to Preview – kit September 19, 2022 07:36 View deployment

dummdidumm approved these changes Sep 19, 2022

View reviewed changes

dummdidumm merged commit 33722bd into sveltejs:master Sep 19, 2022

github-actions bot mentioned this pull request Sep 19, 2022

Version Packages (next) #6881

Merged

david-plugge mentioned this pull request Sep 19, 2022

improve readability of generated types #6883

Merged

5 tasks

david-plugge mentioned this pull request Sep 19, 2022

export ValidationError interface #6885

Merged

5 tasks

Uh oh!

[fix] #6823 - some properties in type of object returned from ActionData is missing #6869

[fix] #6823 - some properties in type of object returned from ActionData is missing #6869

Uh oh!

Conversation

williamviktorsson commented Sep 17, 2022 • edited by dummdidumm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

Uh oh!

changeset-bot bot commented Sep 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

williamviktorsson commented Sep 17, 2022

Uh oh!

repsac-by commented Sep 18, 2022

Uh oh!

williamviktorsson commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

williamviktorsson commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-plugge commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdcarson commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdcarson commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

williamviktorsson commented Sep 18, 2022

Uh oh!

williamviktorsson commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022

Uh oh!

dummdidumm commented Sep 18, 2022

Uh oh!

This comment was marked as off-topic.

cdcarson commented Sep 18, 2022

Uh oh!

dummdidumm commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-plugge commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 18, 2022

Uh oh!

williamviktorsson commented Sep 18, 2022

Uh oh!

williamviktorsson commented Sep 18, 2022

Uh oh!

david-plugge commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dummdidumm commented Sep 19, 2022

Uh oh!

williamviktorsson commented Sep 19, 2022

Uh oh!

cdcarson commented Sep 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

williamviktorsson commented Sep 17, 2022 •

edited by dummdidumm

Loading

changeset-bot bot commented Sep 17, 2022 •

edited

Loading

david-plugge commented Sep 18, 2022 •

edited

Loading

williamviktorsson commented Sep 18, 2022 •

edited

Loading

david-plugge commented Sep 18, 2022 •

edited

Loading

david-plugge commented Sep 18, 2022 •

edited

Loading

cdcarson commented Sep 18, 2022 •

edited

Loading

dummdidumm commented Sep 18, 2022 •

edited

Loading

david-plugge commented Sep 19, 2022 •

edited

Loading