some properties in type of object returned from ActionData is missing #6823
Comments
|
Could you clarify if this is a typing issue (the types are incorrect) or a runtime issue (something isn't showing up in |
This is a typing issue |
|
I am experiencing the same. I would much more prefer having the ActionData type to be: type ActionData = {
first: string | undefined,
second: string | undefined,
third: string | undefined,
fourth: string |undefined
} |
I very much prefer the unions as they are now. While the auto generated types are useful, you'll have a way better experience if you add a little bit of typing yourself. E.g., you could type the payload of invalid responses as |
|
My current use case: type ActionData =
| { target: 'email', message: 'incorrect format'}
| { target: 'email', message: 'not exist'}
| { target: 'trials', message: 'requested too many times', remaining: '30'} // indicating needed to wait 30 seconds to try againBut with current sveltekit version, the So I suspect it is a bug as it 'eats up' some types. And that occurs at the return type of the default. I do like the idea of discriminative union as it would be handled beautifully as we use it with conditionals. |
Would you happen to have an example of how to use the unions as they are now? The example from the docs will not pass typing checks when the unions are the way they are. https://kit.svelte.dev/docs/form-actions#anatomy-of-an-action-validation-errors Attached below the errors from running the docs example.
|
I think this should have worked. They aren’t working only because maybe there is a bug. |
|
Here is a workaround i got from https://fettblog.eu/typescript-union-to-intersection/ src/lib/types.ts export type UnionToIntersection<T> = (T extends any ? (x: T) => any : never) extends (x: infer R) => any
? R
: never;src/routes/+page.svelte <script lang="ts">
import type { ActionData } from './$types';
import type { UnionToIntersection } from '$lib/types';
export let form: UnionToIntersection<ActionData>;
</script> |
|
It would seem that PageData types are handling this correctly when data of different types could be passed. |
|
Types aren't missing. ActionData is an union so you'll need to typeguard type ActionData =
| { target: 'email', message: 'incorrect format'}
| { target: 'email', message: 'not exist'}
| { target: 'trials', message: 'requested too many times', remaining: '30'};
function action(): ActionData | undefined {
return { target: 'trials', message: 'requested too many times', remaining: '30' };
}
const form = action();
console.log(form?.target, form?.message); // This works as they're common in all types of union
// console.log(form?.remaining); // This will throw an error.
if (form?.target == 'trials') {
// This works.
console.log(form.remaining);
}The docs don't use typeguards so it throws type errors as well. {#if form?.missing}<p class="error">No user found with this email</p>{/if}vs (is typesafe but verbose) #{if form && 'missing' in form && form.missing}.... |
But how would it work in the action in +page.server.ts ? |
|
I've tried to make a better example to highlight the issue. If I have a form Action with this code: if (condition)
return { message: "123", info: "abc" }
else
return { message: "123", something: 123 }I get the following type (correct): (alias) type ActionData = {
message: string;
info: string;
something?: undefined;
} | {
message: string;
something: number;
info?: undefined;
}But using the invalid helper function: if (condition)
return invalid(400,{ message: "123", info: "abc" })
else
return invalid(400,{ message: "123", something: 123 })I get this type which is harder to work with and inconsistent with how the load function return types are generated as well: (alias) type ActionData = {
message: string;
info: string;
} | {
message: string;
something: number;
}Are there perks to this or is it a bug? In the case where it's intended, would it be easy to opt out of it by let's say not using the invalid helper function? |
As you said the invalid is a helper function and you may opt out of it. The reason to use it is to differentiate from successful submit or invalid submit thus better dev experience. For the typings, both cases are not bug as they are computed by typescript. The bug I am illustrating in the issue is instead a property being missing completely from the generated ActionData type. You may clone the repo I posted and try to see the type with vscode. |
|
I cloned your repo and had a look and I realize we are trying to highlight two separate issues. Your issue is that some potential return types are missing entirely in the generated types description. My issue is that the algorithm for generating return types behaves differently on objects returned using the invalid helper function, compared to successfully returned objects. I am now certain these are related issues I cloned your repo and if I do this instead: if (url.searchParams.get('v') === '0') return { first: '1' };
if (url.searchParams.get('v') === '1') return { first: 1, second: 'second' };
if (url.searchParams.get('v') === '2') return { first: '3', second: 'second' };
if (url.searchParams.get('v') === '3') return { first: {}, second: 'second' };
if (url.searchParams.get('v') === '4') return { third: 3 };
if (url.searchParams.get('v') === '5') return { third: 'third' };
if (url.searchParams.get('v') === '6') return { fourth: 'fourth' };The types are properly generated. If I do this: if (url.searchParams.get('v') === '0') return invalid(400, { first: '1' });
if (url.searchParams.get('v') === '1') return invalid(400, { first: 1, second: 'second' });
if (url.searchParams.get('v') === '2') return invalid(400, { first: '3', second: 'second' });
if (url.searchParams.get('v') === '3') return invalid(400, { first: {}, second: 'second' });
if (url.searchParams.get('v') === '4') return invalid(400, { third: 3 });
if (url.searchParams.get('v') === '5') return invalid(400, { third: 'third' });
if (url.searchParams.get('v') === '6') return invalid(400, { fourth: 'fourth' });They are not . I believe the issue you are seeing when mixing both successful and invalid errors are due to the invalid errors not being handled properly. If types were generated for the invalid returns in the same matter the regular return objects have their types returned, both of our issues would be resolved. |
|
I'm having pretty much the same general set of issues. I can get the generated types to work, but it (a) requires the same amount of "custom" typing on my part as if the generated types were not a thing, or (b) even more "custom" typing (unions and so on to handle the different cases.) I'm wondering what the use case is for generated action types. For Net: as of now, the generated types are just getting in the way. |
If you enjoy the PageData object, it seems to me like you'll should find that you'll enjoy the same for ActionData if you simply omit the existence of the |
@williamviktorsson Thanks. I understand. But Kit is relying on the (private, unexported) In turn, the status code set on kit/packages/kit/src/runtime/client/client.js Line 1205 in e512ddb ...which is both used by
There's a strong case for generating
Even if the generated types could be fixed and/or documented, I'd much rather create my own types to share between the |
|
Yep, I've been looking at the code and types for ValidationError class for the past few hours and the only solution I can see is to go back to the previous return {status, data} but I'm certain there's a middle ground. A large issue now is that the super simple example from the form actions docs doesn't even work with the current types being generated using |
There actually is a way to solve it without the need of throwing errors and redirects. The symbol is needed to differentiate between a import type { ServerLoadEvent } from '@sveltejs/kit';
const redirectSymbol: unique symbol = Symbol.for('REDIRECT');
class KitRedirect {
readonly [redirectSymbol] = true;
readonly status: number;
readonly location: string;
constructor(status: number, location: string) {
this.status = status;
this.location = location;
}
}
const errorSymbol: unique symbol = Symbol.for('ERROR');
class KitError {
readonly [errorSymbol] = true;
readonly status: number;
readonly message: string;
constructor(status: number, message: string) {
this.status = status;
this.message = message;
}
}
function redirect(status: number, location: string) {
return new KitRedirect(status, location);
}
function error(status: number, message: string) {
return new KitError(status, message);
}
const load = async ({ locals, params }: ServerLoadEvent) => {
if (params.id !== 'a' && params.id !== 'b') {
return error(404, 'Not found');
}
if (!locals.user) {
return redirect(303, '/signin');
}
return {
posts: [
{ id: 1, title: 'Test 1' },
{ id: 2, title: 'Test 2' }
]
};
};
type LoadData = Exclude<Awaited<ReturnType<typeof load>>, KitRedirect | KitError>; |
|
I´ve found a way to correctly extract the types from actions: const actions = {
default: async ({ request }: RequestEvent) => {
const formData = await request.formData();
const email = formData.get('email');
if (!email) {
return invalid(400, {
emailMissing: true
});
}
return {
test: 1
};
}
};
type UnpackValidationError<T> = T extends KitInvalid<infer X> ? X : T;
type UnionToIntersection<T> = (T extends any ? (x: T) => any : never) extends (x: infer R) => any
? R
: never;
type OptionalUnion<U, A = Partial<Record<keyof UnionToIntersection<U>, never>>> = U extends unknown
? Omit<A, keyof U> & U
: never;
type ExtractActionReturnType<T extends (...args: any) => any> = OptionalUnion<
UnpackValidationError<Awaited<ReturnType<T>>>
>;
type ExtractActionData<T extends Record<string, (...args: any) => any>> = {
[Key in keyof T]: ExtractActionReturnType<T[Key]>;
}[keyof T];
// this is now 'correctly' typed
type ActionData = ExtractActionData<typeof actions>; |
|
@david-plugge That's interesting and great. It might make things less confusing. But I would want something DRY-er, where one doesn't have to repeatedly wrap // src/routes/profile/edit/+page.server.ts
export const load = async (event) => {
// returns user, throws redirect
const user = await gateAuthenticated(event);
return { user };
}
export const actions = {
default: async (event) => {
// returns user, throws redirect
const user = await gateAuthenticated(event);
// etc...
}
}This is a really well-known, useful, testable, DRY pattern. In SvelteKit, though, it's been turned on its head, IMO to suit the needs of the framework rather than the needs of the developer. I like SvelteKit enough not to run away, and I know I can roll my own wrapper(s) to allow me to write code as above. Other folks may not feel the same. |
|
Good point, that would end up in a if-return hell since you can´t redirect or error directly from a helper method.
What exactly don't you like about the current approach and how do you think it can be improved? |
This works! Happy to see it. Allows using the invalid function whilst also returning the proper types. I see no real reason not to make a PR out of it. Solves my issue and @harrylaulau and the broken examples from the docs. This is how I adapted your code to 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<keyof UnionToIntersection<U>, never>>> = U extends unknown
? Omit<A, keyof U> & U
: never;
type UnionToIntersection<T> = (T extends any ? (x: T) => any : never) extends (x: infer R) => any
? R
: never; |
|
@williamviktorsson Feel free to create one, or i´ll do it tomorrow :) |
That phrase of mine is stronger than I really meant it to be. Generally I think SvelteKit tends to try to do too much, poach too far into userland, which tends to make the DX less nice over time, unintentionally. Features that sound really great end up dictating weirdness on the developer side (like the throw/return semantic I so intensely dislike.)
For actions, I don't think there's a strong case for generated types. It's not too much to ask the developer to write types by hand in typescript or in JSDoc annotations. It would avoid a lot of hoop-jumping, both on the framework side and the developer side. It would be straightforward and much less prone to error. |
…ata is missing (#6869) Fixes #6823 Fixes #6822 Co-authored-by: William Viktorsson <[email protected]> Co-authored-by: williamv <[email protected]>
Describe the bug
This issue is quite hard to explain by words, I would try my best to describe it precisely. But it think it would be the best to look at the screenshots / code below or the repo.
Issue:
When the invalid function is used as a return of multiple conditionals, AND if they share properties of the same type, the other properties that they don't share would be missing. It is even stranger when an object is the value of the property.
+page.server.ts+page.svelteReproduction
https://github.com/harrylaulau/sveltekittest
Logs
No response
System Info
System: OS: macOS 12.6 CPU: (10) arm64 Apple M1 Max Memory: 324.70 MB / 32.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 16.15.0 - ~/.nvm/versions/node/v16.15.0/bin/node Yarn: 1.22.19 - ~/Library/pnpm/yarn npm: 8.12.1 - ~/.nvm/versions/node/v16.15.0/bin/npm Watchman: 2022.08.15.00 - /opt/homebrew/bin/watchman Browsers: Chrome: 105.0.5195.125 Safari: 16.0 npmPackages: @sveltejs/adapter-auto: next => 1.0.0-next.74 @sveltejs/kit: next => 1.0.0-next.483 svelte: ^3.44.0 => 3.50.1 vite: ^3.1.0 => 3.1.0Severity
serious, but I can work around it
Additional Information
It seems to me that the bug occurs from the return type of
default.+page.server.tsproxy+page.server.tsThe text was updated successfully, but these errors were encountered: