-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Validated forms #14383
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
Validated forms #14383
Conversation
🦋 Changeset detectedLatest commit: 37c10c9 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 |
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.
Looks good so far! Some notes / blockers:
- would be nice to have a way to tell "hide this from output" (e.g. password)
- we should coerce boolean/number via some opt in mechanism, else you'll often parse twice. The types should allow it, too since you can do it yourself today, but it's not pretty:
v.pipe(
v.unknown(),
v.transform((value) => {
if (typeof value !== 'string') {
return value;
}
if (value === '') {
return undefined;
}
return Number(value.trim().replace(/,/g, ''));
}),
v.number()
)
Maybe both of these can be solved via some opionated name syntax: <input type="password" name="hidden:password
and <input type="number" name="number:count" >
(you can concatenate them)
Also you cannot use files with this yet, as the submission tries to return the file in inputs
, and that fails when devalue tries to stringify it. We should omit them from the response I think.
I also twice got "excessively deep" type errors, which makes me a bit nervous, but it's not reproducible.
isn't coercion as simple as this? v.pipe(v.string(), v.regex(/^\d+$/), v.transform((n) => +n))
I've searched in vain for any authoritative advice that this is necessary, rather than paranoid. Do we truly need it? Yeah omitting files altogether is probably the best approach |
I take back the coercion part - it is easy enough in most validators, and it also solves my adjacent "the types should allow you to pass in more object shapes" request, because if the coercion is done via validators then the shape of only allowing strings or files is correct. We should have something about it in the docs though, gonna work on that. The "excessively deep" error indeed occured from |
…ll result in infinite recursion type errors
…o validated-forms
Really excited about this PR - can't wait to get my hands on it, regarding your comment on whether or not we could do without do you have any suggestion to how we could protect unique indexes such as a user email, I confess I can't see an elegant way to do it without a transaction in the remote function // signup.remote.ts
const schema = z.object({
email: z.string().refine(async (email) => {
const { locals: { db } } = getRequestEvent();
const emailIsTaken = db.select().from(db.users).where(...)
return !!emailIsTaken;
}, ['This email is already taken'])
})
export const signup = form(schema, async ({email} => {
// Inserting email here could still result in an index violation
// I can't see a way to do it without resorting to the invalid api
db.transaction((tx) => {
const emailIsTaken = db.select().from(users)
if (emailIsTaken) {
invalid('email', 'this email is already taken')
}
})
redirect(...)
}) |
Great question — I'll confess I hadn't really considered the race condition case. To be honest I would probably just live with the extremely slim chance of a 500 (what are the odds of someone registering the same email address at the exact same moment?) but that doesn't really work if you're building the order form for hotcakes.com. One of the reasons I wasn't too confident about the const result = await MySchema.safeParse(data);
if (!result.success) {
invalid(result.issues);
} ...but you also want to be able to do this... db.transaction((tx) => {
const emailIsTaken = db.select().from(users)
if (emailIsTaken) {
invalid(???)
}
// ...
}) ...and ideally you wouldn't have to conform to the But a few commits ago we decided to redact everything inside each issue except the |
Implements #14288
might not be able to flatten keys recursively, because TypeScript complainsfigured it out, I thinkissues
input
preflight(...)
validate()
enhance
callback, rather than theFormData
objectOn second thoughts, I think we should ship what we have and see if we truly need this API, since it's a slightly awkward oneinvalid()
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. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits