-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: better remote form field interactions #14481
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
Conversation
Shortly after merging enhanced form validation we noticed that we can do better with respects to interacting with the form, specifically `field()/issues/input`. This removes those properties in favor of a new `fields` property which makes interacting with the form much easier. It's using a proxy under the hood.
🦋 Changeset detectedLatest commit: e1dc8b8 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 |
us to everyone who just finished refactoring experimental version 1 to experimental version 2 after we merged #14383 ![]() |
TODO: doesn't work for nested due to Object.create(null)
hey, i don't know if the goal is just validate the form, but i was think if the form somehow could accept a initial data, i know there's the |
I like this way of doing it better than relying on string for handling the nesting. A high priority for me is how easy it is to pass along subsets of a form to components so that it is easy to write reusable components or to split a big form into multiple steps. If each branch / leaf could have access to creating a custom component could be very easy. And as you wrote also have methods to check if the passed in form prop is valid (or any of the leafs) Looking forwards to see what you come up with :) Ps. Angular seems to also be working on a new version. might find some inspiration there? |
Some observations while working on this:
|
As part of sveltejs/kit#14481 we discovered that deriveds created within reactions and reading from them in that same reaction is actually useful in some cases, as such a use case we couldn't imagine yet in #15564 has appeared. We think it's ultimately better to rerun on those cases, so we're going to make this change in async mode (that way the behavior doesn't change unless you have enabled the experimental flag)
…#16823) * fix: depend on reads of deriveds created within reaction (async mode) As part of sveltejs/kit#14481 we discovered that deriveds created within reactions and reading from them in that same reaction is actually useful in some cases, as such a use case we couldn't imagine yet in #15564 has appeared. We think it's ultimately better to rerun on those cases, so we're going to make this change in async mode (that way the behavior doesn't change unless you have enabled the experimental flag) * fix tests
This is feeling really nice. A few notes:
Will start working through these |
…lds when reloading
The maintainers had a call earlier and decided a few things:
We also talked about input masks. For now we're not going to put anything related to masks in this API, as it's the sort of thing that is probably best done with attachments |
What would these do? Or did you mean |
The preview deployment is failing because it's using the previous version of SvelteKit and the types don't match. I've verified that the docs build locally. |
(Side-note but I'm starting to think remote functions need their own top-level section, the docs are getting quite large) |
packages/kit/src/exports/public.d.ts
Outdated
type FormField<ValueType> = | ||
NonNullable<ValueType> extends string | string[] | number | boolean | File | File[] | ||
? FormFieldMethods<ValueType> & { | ||
/** | ||
* Returns an object that can be spread onto an input element with the correct type attribute, | ||
* aria-invalid attribute if the field is invalid, and appropriate value/checked property getters/setters. | ||
* @example | ||
* ```svelte | ||
* <input {...myForm.fields.myString.as('text')} /> | ||
* <input {...myForm.fields.myNumber.as('number')} /> | ||
* <input {...myForm.fields.myBoolean.as('checkbox')} /> | ||
* ``` | ||
*/ | ||
as<T extends ValidInputTypesForValue<ValueType>>( | ||
...args: AsArgs<T, ValueType> | ||
): InputElementProps<T>; | ||
} | ||
: FormFieldMethods<ValueType> & { | ||
/** Validation issues belonging to this or any of the fields that belong to it, if any */ | ||
allIssues(): RemoteFormIssue[] | undefined; | ||
}; | ||
|
||
/** | ||
* Recursive type to build form fields structure with proxy access | ||
*/ | ||
type FormFields<T> = | ||
WillRecurseIndefinitely<T> extends true | ||
? RecursiveFormFields | ||
: NonNullable<T> extends string | number | boolean | File | ||
? FormField<T> | ||
: T extends Array<infer U> | ||
? FormField<T> & { [K in number]: FormFields<U> } | ||
: FormField<T> & { [K in keyof T]-?: FormFields<T[K]> }; | ||
|
||
// By breaking this out into its own type, we avoid the TS recursion depth limit | ||
type RecursiveFormFields = FormField<any> & { [key: string]: RecursiveFormFields }; |
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.
If these types were exported, would it be possible to create custom components that wrap individual fields?
In this simple example:
FormField
AsArgs
ValidInputTypesForValue
<script lang="ts">
type ValueType = $$Generic<string | string[] | number | boolean | File | File[]>;
type InputType = $$Generic<ValidInputTypesForValue<ValueType>>;
interface Props extends HTMLInputAttributes {
label: string;
field: FormField<ValueType>;
as: AsArgs<InputType, ValueType>;
}
const { label, field, as, ...rest }: Props = $props();
</script>
<label>
<span>{label}</span>
<input {...rest} {...field.as(...as)}>
<!-- Errors -->
</label>
<Input field={form.fields.title} as={["text"]} />
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've renamed and exposed a couple of things and got as far as this:
<script lang="ts" generics="V extends RemoteFormFieldValue, T extends RemoteFormFieldType<V>">
import type { RemoteFormField, RemoteFormFieldType, RemoteFormFieldValue } from '@sveltejs/kit';
import type { HTMLInputAttributes } from 'svelte/elements';
type ShouldHaveValue<V, T> = T extends 'radio'
? true
: T extends 'checkbox'
? V extends string[]
? true
: false
: false;
type Props =
ShouldHaveValue<V, T> extends true
? {
label: string;
field: RemoteFormField<V>;
type: T;
value: string;
}
: {
label: string;
field: RemoteFormField<V>;
type: T;
};
const {
label,
field,
type,
// @ts-expect-error
value,
...rest
}: Omit<HTMLInputAttributes, 'type' | 'value'> & Props = $props();
// @ts-expect-error
const attributes = $derived(field.as(type, value));
</script>
<label>
<span>{label}</span>
<input {...rest} {...attributes} />
<!-- Errors -->
</label>
Maybe exposing AsArgs
and expecting as={["text"]}
would allow us to get rid of the ts-expect-error
comments it but it feels rather clunky — would be much nicer to do this:
<Input field={form.fields.title} as="text" />
<Input field={form.fields.options} as="radio" value="foo" />
<Input field={form.fields.options} as="radio" value="bar" />
<Input field={form.fields.options} as="radio" value="baz" />
Which does work with the above, with one caveat — no autocomplete for as
, just red squigglies after the fact if you get it wrong. Quite involved though. Would welcome suggestions for improvements
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'll see if I can improve it when I get my hands on it.
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.
Another approach could be to have separate <Radio>
and <Checkbox>
components, rather than shoving everything into <Input>
. Might make things simpler
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.
That's certainly a good approach. I copied the relevant types to a TS file and think I got autocomplete with this:
function Input<V extends RemoteFormFieldValue>(
props: {
label: string;
field: RemoteFormField<V>;
} & (V extends string[]
? { type: "checkbox"; value: string } | { type: "select multiple" }
: V extends string
? { type: "radio"; value: string } | { type: Exclude<RemoteFormFieldType<V>, "radio"> }
: { type: RemoteFormFieldType<V> }
)
) {
return props;
}
@saturnonearth one thing at a time, please. ask us again in a month |
Co-authored-by: Patrick <Patrick@ShowYou.us>
… remote-form-api-tweaks
We just landed an enhanced version of the remote
form
function in SvelteKit. It makes it possible to pass a schema, have that auto-transformFormData
into an object, and have properties such asinput
andissues
for getting the form's current value or schema validation issues of a specific field.This works nicely for the simple case where you're only dealing with an object depth of 1:
But as soon as you work with array leafs or nested objects, it becomes a bit cumbersome/weird:
You could extract the repeated string access into a
{@const ...}
and reuse that but you can see it's not that pretty. It also has problems around leaf arrays which would befoo.field('array[]')
forfield()
butfoo.issues.array / foo.input.array
for the others, which is kinda inconsistent and makes the{@const ...}
extraction not work in all cases; the alternative of always requiringarray[]
looks and feels too weird.So we kinda got thinking - could we leverage proxies instead and use regular object notation with method names at the leafs?
It would work like this:
Simple case from above:
Nested/leaf example from above:
Again you could in both examples extract paths into
{@const ...}
, but this time in a consistent, safe manner.To me this feels a lot nicer to write as I'm not munging together strings, I can also rely on autocomplete a lot better and discover the form's shape as I go instead of having to type out everything at once.
This will also offer a lot more flexibility with regards how people can build component libraries/abstractions around it.
We can also get more creative with what methods we want to have on there besides
name()/issues()/value()
. We could haveinitial()/validate()/reset()/valid()
etc - basically anything that works for both objects and leaf.valid()
in particular could mean "everything below me is valid", something that is really hard to pull off with the current design (you essentially would have to do something likearia-invalid={somehowGetListOfAllStrings().some(s => foo.issues[s])
), with the proposed design you'd just doaria-invalid={!foo.fields.field.i.care.about.valid()}
Under the hood removes those properties in favor of a new
fields
property which makes interacting with the form much easier. It's using a proxy under the hood.WIP:
.value()
(did this even work in the old world?)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:
.