Skip to content
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

Support for @validateOn/@revalidateOn options #25

Merged
merged 8 commits into from
Feb 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ember-headless-form/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"dependencies": {
"@babel/runtime": "^7.20.7",
"@embroider/addon-shim": "^1.0.0",
"@ember/test-waiters": "^3.0.2",
"@embroider/util": "^1.9.0",
"tracked-built-ins": "^3.1.0"
},
Expand Down Expand Up @@ -57,6 +58,7 @@
"@types/ember__debug": "^4.0.0",
"@types/ember__engine": "^4.0.0",
"@types/ember__error": "^4.0.0",
"@types/ember__modifier": "^4.0.3",
"@types/ember__object": "^4.0.0",
"@types/ember__polyfills": "^4.0.0",
"@types/ember__routing": "^4.0.0",
Expand Down
8 changes: 3 additions & 5 deletions ember-headless-form/src/components/-private/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export type FieldValidateCallback<
* Internal structure to track used fields
* @private
*/
export interface FieldData<
export interface FieldRegistrationData<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What IS FieldRegistrationData...like why did this get renamed? I think I don't have the clearest mental model yet of everything going on and how it fits together.

Maybe I could use some JSDoc for this interface and the one below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was splitting this up into two concerns, that's why I renamed this.

So there is information passed form the field component to the parent form component via the registerField action. Basically the field register itself with its parent form, so the form has data available that the dev passed to the field. Currently this is just the field level validation callback, which the form needs to have available. This is what the FieldRegistrationData contains.

Then in this PR there is a new FieldData class being added, which gets created when registering the field, so it uses FieldRegistrationData, but it also contains more than that (the validationEnabled tracked property), which the field does not need to care about.

So that's why we have these two different but related concepts now.

As mentioned earlier, I added a bunch of JSDoc comments, also here, so hopefully that becomes more clear now!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> {
Expand All @@ -69,7 +69,7 @@ export interface FieldData<
export type RegisterFieldCallback<
DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> = (name: KEY, field: FieldData<DATA, KEY>) => void;
> = (name: KEY, field: FieldRegistrationData<DATA, KEY>) => void;

export type UnregisterFieldCallback<
DATA extends FormData,
Expand All @@ -79,6 +79,4 @@ export type UnregisterFieldCallback<
/**
* Mapper type to construct subset of objects, whose keys are only strings (and not number or symbol)
*/
export type OnlyStringKeys<T extends object> = {
[P in Extract<keyof T, string>]: T[P];
};
export type OnlyStringKeys<T extends object> = Pick<T, keyof T & string>;
13 changes: 11 additions & 2 deletions ember-headless-form/src/components/headless-form.hbs
Original file line number Diff line number Diff line change
@@ -1,11 +1,20 @@
<form ...attributes {{on 'submit' this.onSubmit}}>
{{! ignoring prettier here is need to *not* wrap the modifier usage below into a new line, making @glint-expect-error fail to work 🙈 }}
{{! prettier-ignore }}
<form
...attributes
{{on 'submit' this.onSubmit}}
{{! @glint-expect-error: modifier helper not supported, see https://github.com/typed-ember/glint/issues/410 }}
{{(if this.fieldValidationEvent (modifier this.on this.fieldValidationEvent this.handleFieldValidation))}}
{{! @glint-expect-error: modifier helper not supported, see https://github.com/typed-ember/glint/issues/410 }}
{{(if this.fieldRevalidationEvent (modifier this.on this.fieldRevalidationEvent this.handleFieldRevalidation))}}
>
{{yield
(hash
field=(component
(ensure-safe-component this.FieldComponent)
data=this.internalData
set=this.set
errors=this.lastValidationResult
errors=this.visibleErrors
registerField=this.registerField
unregisterField=this.unregisterField
)
Expand Down
171 changes: 164 additions & 7 deletions ember-headless-form/src/components/headless-form.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import Component from '@glimmer/component';
import { tracked } from '@glimmer/tracking';
import { assert } from '@ember/debug';
import { on } from '@ember/modifier';
import { action } from '@ember/object';
import { waitFor } from '@ember/test-waiters';

import { TrackedObject } from 'tracked-built-ins';

Expand All @@ -10,11 +12,13 @@ import FieldComponent from './-private/field';
import type { HeadlessFormFieldComponentSignature } from './-private/field';
import type {
ErrorRecord,
FieldData,
FieldRegistrationData,
FieldValidateCallback,
FormData,
FormKey,
FormValidateCallback,
UserData,
ValidationError,
} from './-private/types';
import type { ComponentLike, WithBoundArgs } from '@glint/template';

Expand All @@ -37,28 +41,62 @@ export interface HeadlessFormComponentSignature<DATA extends UserData> {
default: [
{
field: WithBoundArgs<
typeof FieldComponent<FormData<DATA>>,
typeof FieldComponent<DATA>,
'data' | 'set' | 'errors' | 'registerField' | 'unregisterField'
>;
}
];
};
}

/**
* This internal data structure maintains information about each field that is registered to the form by `registerField`.
*/
class FieldData<
DATA extends FormData,
KEY extends FormKey<DATA> = FormKey<DATA>
> {
constructor(fieldRegistration: FieldRegistrationData<DATA, KEY>) {
this.validate = fieldRegistration.validate;
}

/**
* tracked state that enabled a dynamic validation of a field *before* the whole form is submitted, e.g. by `@validateOn="blur" and the blur event being triggered for that particular field.
*/
@tracked validationEnabled = false;

/**
* The *field* level validation callback passed to the field as in `<form.field @name="foo" @validate={{this.validateCallback}}>`
*/
validate?: FieldValidateCallback<DATA, KEY>;
}

export default class HeadlessFormComponent<
DATA extends UserData
> extends Component<HeadlessFormComponentSignature<DATA>> {
FieldComponent: ComponentLike<HeadlessFormFieldComponentSignature<DATA>> =
FieldComponent;

internalData: FormData<DATA> = new TrackedObject(
this.args.data ?? {}
) as FormData<DATA>;
// we cannot use (modifier "on") directly in the template due to https://github.com/emberjs/ember.js/issues/19869
on = on;

/**
* A copy of the passed `@data` stored internally, which is only passed back to the component consumer after a (successful) form submission.
*/
internalData: DATA = new TrackedObject(this.args.data ?? {}) as DATA;

fields = new Map<FormKey<FormData<DATA>>, FieldData<FormData<DATA>>>();

/**
* The last result of calling `this.validate()`.
*/
@tracked lastValidationResult?: ErrorRecord<FormData<DATA>>;

/**
* When this is set to true by submitting the form, eventual validation errors are show for *all* field, regardless of their individual dynamic validation status in `FieldData#validationEnabled`
*/
@tracked showAllValidations = false;

get validateOn(): ValidateOn {
return this.args.validateOn ?? 'submit';
}
Expand All @@ -67,6 +105,43 @@ export default class HeadlessFormComponent<
return this.args.revalidateOn ?? 'change';
}

/**
* Return the event type that will be listened on for dynamic validation (i.e. *before* submitting)
*/
get fieldValidationEvent(): 'focusout' | 'change' | undefined {
const { validateOn } = this;

return validateOn === 'submit'
? // no need for dynamic validation, as validation always happens on submit
undefined
: // our component API expects "blur", but the actual blur event does not bubble up, so we use focusout internally instead
simonihmig marked this conversation as resolved.
Show resolved Hide resolved
validateOn === 'blur'
? 'focusout'
: validateOn;
}

/**
* Return the event type that will be listened on for dynamic *re*validation, i.e. updating the validation status of a field that has been previously marked as invalid
*/
get fieldRevalidationEvent(): 'focusout' | 'change' | undefined {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: I could use some JSDOC here to remind us when this getter gets triggered and why this gets triggered instead of triggering fieldValidation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added, and a bunch of more JSDoc annotations along that!

This was good to call out, thank you! Indeed there are different things playing together here, which were clear to me as I was working on the things directly for many hours, but which might not be obvious on a review or when working in the codebase and not being super familiar with it. So, yeah, I must remind myself to explain things better here!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These comments are super helpful, thank you!

const { validateOn, revalidateOn } = this;

return revalidateOn === 'submit'
? // no need for dynamic validation, as validation always happens on submit
undefined
: // when validation happens more frequently than revalidation, then we can ignore revalidation, because the validation handler will already cover us
validateOn === 'change' ||
(validateOn === 'blur' && revalidateOn === 'blur')
? undefined
: // our component API expects "blur", but the actual blur event does not bubble up, so we use focusout internally instead
revalidateOn === 'blur'
? 'focusout'
: revalidateOn;
}

/**
* Return true if validation has happened (by submitting or by an `@validateOn` event being triggered) and at least one field is invalid
*/
get hasValidationErrors(): boolean {
// Only consider validation errors for which we actually have a field rendered
return this.lastValidationResult
Expand All @@ -76,6 +151,10 @@ export default class HeadlessFormComponent<
: false;
}

/**
* Call the passed validation callbacks, defined both on the whole form as well as on field level, and return the merged result for all fields.
*/
@waitFor
async validate(): Promise<ErrorRecord<FormData<DATA>> | undefined> {
let errors: ErrorRecord<FormData<DATA>> | undefined = undefined;

Expand Down Expand Up @@ -106,11 +185,46 @@ export default class HeadlessFormComponent<
return Object.keys(errors).length > 0 ? errors : undefined;
}

/**
* Return a mapping of field to validation errors, for all fields that are invalid *and* for which validation errors should be visible.
* Validation errors will be visible for a certain field, if validation errors for *all* fields are visible, which is the case when trying to submit the form,
* or when that field has triggered the event given by `@validateOn` for showing validation errors before submitting, e.g. on blur.
*/
get visibleErrors(): ErrorRecord<FormData<DATA>> | undefined {
if (!this.lastValidationResult) {
return undefined;
}

const visibleErrors: ErrorRecord<FormData<DATA>> = {};

for (const [field, errors] of Object.entries(this.lastValidationResult) as [
FormKey<FormData<DATA>>,
ValidationError<FormData<DATA>[FormKey<FormData<DATA>>]>[]
][]) {
if (this.showErrorsFor(field)) {
visibleErrors[field] = errors;
}
}

return visibleErrors;
}

/**
* Given a field name, return if eventual errors for the field should be visible. See `visibleErrors` for further details.
*/
showErrorsFor(field: FormKey<FormData<DATA>>): boolean {
return (
this.showAllValidations ||
(this.fields.get(field)?.validationEnabled ?? false)
);
}

@action
async onSubmit(e: Event): Promise<void> {
e.preventDefault();

this.lastValidationResult = await this.validate();
this.showAllValidations = true;

if (!this.hasValidationErrors) {
this.args.onSubmit?.(this.internalData);
Expand All @@ -126,15 +240,15 @@ export default class HeadlessFormComponent<
@action
registerField(
name: FormKey<FormData<DATA>>,
field: FieldData<FormData<DATA>>
field: FieldRegistrationData<FormData<DATA>>
): void {
assert(
`You passed @name="${String(
name
)}" to the form field, but this is already in use. Names of form fields must be unique!`,
!this.fields.has(name)
);
this.fields.set(name, field);
this.fields.set(name, new FieldData(field));
}

@action
Expand All @@ -146,4 +260,47 @@ export default class HeadlessFormComponent<
set<KEY extends FormKey<FormData<DATA>>>(key: KEY, value: DATA[KEY]): void {
this.internalData[key] = value;
}

/**
* Handle the `@validateOn` event for a certain field, e.g. "blur".
* Associating the event with a field is done by looking at the event target's `name` attribute, which must match one of the `<form.field @name="...">` invocations by the user's template.
* Validation will be triggered, and the particular field will be marked to show eventual validation errors.
*/
@action
async handleFieldValidation(e: Event): Promise<void> {
const { target } = e;
const { name } = target as HTMLInputElement;

if (name) {
const field = this.fields.get(name as FormKey<FormData<DATA>>);

if (field) {
this.lastValidationResult = await this.validate();
field.validationEnabled = true;
}
} else {
// @todo how to handle custom controls that don't emit focusout/change events from native form controls?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DX should be improved here by emitting a warning, by a future PR (see PR description) that provides additional primitives for better custom controls support, and mentions these primitives in the warning. See keeping this as-is for now...

}
}

/**
* Handle the `@revalidateOn` event for a certain field, e.g. "blur".
* Associating the event with a field is done by looking at the event target's `name` attribute, which must match one of the `<form.field @name="...">` invocations by the user's template.
* When a field has been already marked to show validation errors by `@validateOn`, then for revalidation another validation will be triggered.
*
* The use case here is to allow this to happen more frequently than the initial validation, e.g. `@validateOn="blur" @revalidateOn="change"`.
*/
@action
async handleFieldRevalidation(e: Event): Promise<void> {
const { target } = e;
const { name } = target as HTMLInputElement;

if (name) {
if (this.showErrorsFor(name as FormKey<FormData<DATA>>)) {
this.lastValidationResult = await this.validate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interacting with this after an await isn't safe 🙈

I keep meaning to work on this lint but... priorities 🙃

be sure to if (isDestroyed(this) || isDestroying(this)) return after an await

Copy link
Contributor Author

@simonihmig simonihmig Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! So I can certainly add isDestroy{ing,ed} guards here, even if it's just for code style or best practices.

But I'd first like to understand why this could be a problem here, i.e. what is unsafe about it?

So when the async action still has a pointer to this while the component that this refers to is already "destroyed" (unmounted), then this will still exist. There is no way it could be "gone", because that's how Garbage Collection works: here our async action is basically the retainer for that object preventing GC.

So it exists, but it could be in a "destroyed" state (as far as Ember is concerned). In classic Ember, a call to set(this,...) or this.set() would fail, so yes, that would be bad. But in Octane, would mutating a tracked property be so bad? I don't think anything will throw here, right? And when our form is already "unmounted" (in React's terminology, do we have a better term, unrendered?), this tracked property will just not be consumed anymore, so also not cause rerenders, but that should be all, no further harm caused, or am I missing something?

Compare that to this comment here on the linting issue...

Loosely related: should we wrap the async methods (that call potentially async validators) with waitFor() from @ember/test-waiters, so there are able to cover async things happening that Ember does not auto-detect as async? I would think so, yes?

Copy link
Contributor

@NullVoxPopuli NullVoxPopuli Jan 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think so, yes?

yeah, that's a good way to solve this whole class of problem.

what is unsafe about it?

you raise some good points, and I'm going to have to do some concrete research with shareable examples before continuing on with the lint 😅
and the lint may need to be renamed to no-unsafe-container-access or something like that, if it's just accessing services that is the problem in this situation, since service instantiation is lazy

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accessing services that is the problem in this situation, since service instantiation is lazy

Oh right, that can be problematic indeed!

Copy link
Contributor Author

@simonihmig simonihmig Feb 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I added @waitFor to cover async validations, and made the test cover async validations properly (i.e. they were indeed failing without that @waitFor), see this commit

Regarding the isDestroy{ing,ed} topic, should we address this here and now, or eventually iterate on it when it's more clear how to address this?

In other words: is this good to merge? @NullVoxPopuli @nicolechung

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're good to merge.

isDetroy{ing,ed}, I think I want to iterate on the lint, so we can have a definite error case to match on, and then provide an autofix as well

}
} else {
// @todo how to handle custom controls that don't emit focusout/change events from native form controls?
}
}
}
15 changes: 14 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 6 additions & 1 deletion test-app/app/templates/index.hbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
<HeadlessForm @data={{this.data}} @onSubmit={{this.doSomething}} as |form|>
<HeadlessForm
@data={{this.data}}
@validateOn='blur'
@onSubmit={{this.doSomething}}
as |form|
>
<form.field @name='name' as |field|>
<field.label>Name</field.label>
<field.input />
Expand Down
Loading