- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 2.1k
          breaking: invalid now must be imported from @sveltejs/kit
          #14768
        
          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
base: main
Are you sure you want to change the base?
Conversation
TypeScript kinda forced our hand here - due to limitations of control flow analysis it does not detect the `never` return type for anything else than functions that are used directly (i.e. passing a function as a parameter doesn't work unless you explicitly type it); see microsoft/TypeScript#36753 for more info. This therefore changes `invalid` to be a function that you import just like `redirect` or `error`. A nice benefit of this is that you'll no longer have to use the second parameter passed to remote form functions to construct the list of issues in case you want to create an issue for the whole form and not just a specific field. Closes #14745
| 🦋 Changeset detectedLatest commit: 0cb5edc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
 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 | 
| 'invalid() should now be imported from @sveltejs/kit to throw validaition issues. ' + | ||
| 'Keep using the parameter (now named issue) provided to the form function only to construct the issues, e.g. invalid(issue.field("message")). ' + | 
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.
| 'invalid() should now be imported from @sveltejs/kit to throw validaition issues. ' + | |
| 'Keep using the parameter (now named issue) provided to the form function only to construct the issues, e.g. invalid(issue.field("message")). ' + | |
| '`invalid` should now be imported from `@sveltejs/kit` to throw validation issues. ' + | |
| 'The second parameter provided to the form function (renamed to `issue`) is still used to construct issues, e.g. `invalid(issue.field(\'message\'))`. ' + | 
| ### Programmatic validation | ||
| In addition to declarative schema validation, you can programmatically mark fields as invalid inside the form handler using the `invalid` function. This is useful for cases where you can't know if something is valid until you try to perform some action: | ||
| In addition to declarative schema validation, you can programmatically mark fields as invalid inside the form handler using the `invalid` function. This is useful for cases where you can't know if something is valid until you try to perform some action. Just like `redirect` or `error`, `invalid` throws. It expects a list of standard-schema-compliant issues. Use the `issue` parameter for type-safe creation of such issues: | 
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.
| In addition to declarative schema validation, you can programmatically mark fields as invalid inside the form handler using the `invalid` function. This is useful for cases where you can't know if something is valid until you try to perform some action. Just like `redirect` or `error`, `invalid` throws. It expects a list of standard-schema-compliant issues. Use the `issue` parameter for type-safe creation of such issues: | |
| In addition to declarative schema validation, you can programmatically mark fields as invalid inside the form handler using the `invalid` function. This is useful for cases where you can't know if something is valid until you try to perform some action. Just like `redirect` or `error`, `invalid` throws. It expects a list of strings (for issues relating to the form as a whole) or standard-schema-compliant issues (for those relating to a specific field). Use the `issue` parameter for type-safe creation of such issues: | 
| // This will show up on the root issue list | ||
| 'Purchase failed', | ||
| // Creates a `{ message: ..., path: ['qty'] }` object, | ||
| // will show up on the issue list for the `qty` field | ||
| issue.qty(`we don't have enough hotcakes`) | 
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.
I know that the previous example didn't illustrate root-level issues, but I think that showing both like this makes things more confusing rather than less — it suggests that you're supposed to add a root-level issue alongside a field-level issue which I would definitely consider unusual. I think we're better off reverting, and relying on the prose above
| // This will show up on the root issue list | |
| 'Purchase failed', | |
| // Creates a `{ message: ..., path: ['qty'] }` object, | |
| // will show up on the issue list for the `qty` field | |
| issue.qty(`we don't have enough hotcakes`) | |
| invalid.qty(`we don't have enough hotcakes`) | 
| * ```ts | ||
| * import { invalid } from '@sveltejs/kit'; | ||
| * import { form } from '$app/server'; | ||
| * import * as v from 'valibot'; | 
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 can save ourselves some space by making this an import, and I think we should do 'login' rather than 'register' (reasons below)
| * import * as v from 'valibot'; | |
| * import { tryLogin } from $lib/server/auth'; | |
| * import * as v from 'valibot'; | 
| * | ||
| * function tryRegisterUser(name: string, password: string) { | ||
| * // ... | ||
| * } | 
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.
| * | |
| * function tryRegisterUser(name: string, password: string) { | |
| * // ... | |
| * } | 
| * // ... | ||
| * } | ||
| * | ||
| * export const register = form( | 
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.
| * export const register = form( | |
| * export const login = form( | 
| * async ({ name, _password }, issue) => { | ||
| * const success = tryRegisterUser(name, _password); | ||
| * if (!success) { | ||
| * invalid('Registration failed', issue.name('This username is already taken')); | ||
| * } | 
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.
For the same reason as https://github.com/sveltejs/kit/pull/14768/files#r2444740445 I think we should avoid mixing and matching here — this example reads very much as though 'Registration failed' is the title of the issue and 'This username is already taken' is the detail. In reality you would only create the name issue.
Given that the suggestion shows the use of issue.foo(...), I think it makes sense to do the opposite here, and a classic example of a root-level issue is a login failure:
| * async ({ name, _password }, issue) => { | |
| * const success = tryRegisterUser(name, _password); | |
| * if (!success) { | |
| * invalid('Registration failed', issue.name('This username is already taken')); | |
| * } | |
| * async ({ name, _password }) => { | |
| * const success = tryLogin(name, _password); | |
| * if (!success) { | |
| * invalid('Incorrect username or password'); | |
| * } | 
How to migrate
Instead of throwing with the passed in parameter, you now throw via the imported
invalidfunction from@sveltejs/kit.PR description
TypeScript kinda forced our hand here - due to limitations of control flow analysis it does not detect the
neverreturn type for anything else than functions that are used directly (i.e. passing a function as a parameter doesn't work unless you explicitly type it); see microsoft/TypeScript#36753 for more info.This therefore changes
invalidto be a function that you import just likeredirectorerror. A nice benefit of this is that you'll no longer have to use the second parameter passed to remote form functions to construct the list of issues in case you want to create an issue for the whole form and not just a specific field.Closes #14745
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.