-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
export ValidationError interface #6885
export ValidationError interface #6885
Conversation
|
@dummdidumm do you think #6869 (comment) is worth considering? It´s an edge case but i don´t think a helper property like |
I think that's worth considering, yes, but it should probably be a symbol so you don't see it during autocompletion. |
No need to export the classes, the interface is enough for that use case. |
e9af37f
to
19542ae
Compare
Ah sry, misunderstood - it's purely about typing, not about an |
I´d be fine with that but i wonder why the classes should not be exported? |
Just conservatism — the less we expose, the more flexibility we have in future to make changes to how things work under the hood — and a preference for only having one way to do things. In other words it shouldn't be possible to do both of these... import { invalid } from '@sveltejs/kit';
// ...
return invalid(400, {...}); import { ValidationError } from '@sveltejs/kit';
// ...
return new ValidationError(400, {...}); ...because that's needlessly confusing, and the function is more ergonomic and flexible than the class. I'm not sure I understand what's going on with the -Promise<{ success: true } | ValidationError<FormValidationError>>
+Promise<{ success: true } | ReturnType<typeof invalid<400, FormValidationError>>> |
The |
Another argument in favor of exporting the interface is #6933 |
The status as a Generic is actually pretty nice, that way it´s possible to define return types for a specific status code. import type { Actions } from '@sveltejs/kit';
interface ValidationError<S extends number, T extends Record<string, unknown>> {
status: S;
data: T;
}
function invalid<S extends number, T extends Record<string, unknown>>(
status: S,
data: T
): ValidationError<S, T> {
return new ValidationError(status, data);
}
export const actions: Actions = {
default: async ({
url
}): Promise<{ success: true } | ValidationError<400, { valueMissing: true }> | ValidationError<409, { alreadyExists: true }>> => {
if (!requestValid()) {
return invalid(400, {
valueMissing: true
});
}
if (resourceAlreadyExists()) {
return invalid(409, {
alreadyExists: true
});
}
return {
success: true
};
}
}; You can pass |
Before merging this I wanted to first confirm the behavior in #6933 - @ivanhofer could you provide a minimum reproducible (or a code snippet if that's enough)? I can't reproduce this. |
@dummdidumm I have tested it with the changes from this branch and the issue is fixed. This is the content of the stripped down // @ts-nocheck
import { invalid } from '@sveltejs/kit'
import type { Actions } from './$types.js'
export const actions = {
async default() {
return invalid(400)
},
}
; null as any as Actions; It seems this issue only occurs when setting {
"extends": "./.svelte-kit/tsconfig.json",
"compilerOptions": {
"composite": true,
}
} |
Interesting that |
Why is this always included in the generated proxy file? Seems like some nonesense 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to figure out a way to prevent this from showing up in the docs before we can merge this
https://kit-svelte-m4bayg7p6-svelte.vercel.app/docs/modules#do-not-use
What about moving |
A more general solution // private.d.ts
/**
* This doesn't actually exist, it's a way to better distinguish the type
*/
const uniqueSymbol: unique symbol;
export interface UniqueInterface {
readonly [uniqueSymbol]: unknown;
}
// index.d.ts
// Needs to be here, else ActionData will be resolved to unknown - probably because of "d.ts file imports .js file" in combination with allowJs
export interface ValidationError<T extends Record<string, unknown> | undefined = undefined> extends UniqueInterface {
status: number;
data: T;
} |
thank you! |
Allow typing the return type of an action (example)
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. All changesets should bepatch
until SvelteKit 1.0