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

Conversation

simonihmig
Copy link
Contributor

As an iteration over #21, this adds the options to validate and revalidate (when initial validation had errors) not only on submit, but also on blur/focusout and change.

It supersedes #24, and still relies on option 1 discussed in that draft PR. In a future iteration it is expected that we had further primitives for custom controls, like "option 4" of yielding a modifier to capture focusout/change event, or yielding more validation state like field.isValid. But this can all be done on top of this PR.

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...

Copy link
Contributor

@nicolechung nicolechung left a comment

Choose a reason for hiding this comment

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

So far looking GREAT 💯 ...I have a bunch of questions / suggestions...but very excited to see how easy this is to use

: validateOn;
}

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!


assert.true(
validateCallback.calledWith({ ...data, firstName: 'Foo' }),
'@validate is called with form data'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion, nit:'@validate is called with form data' -> '@validate is called with form data on blur'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied most of your assertion suggestions here, and at all the other places were similar things were tested! Thanks!

});
});

module(`@revalidateOn`, function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: maybe this module should be a separate revalidate file...this is getting a bit long

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 have mixed feelings about this, because it is hard to draw the boundaries here for what should go into what testing file, as all these things belong together and can be combined. For example I don't have test coverage here for all the combinations, like @validateOn="blur" @revalidateOn="change". But this would be a good and valid test scenario, and into which test file should be put it then?

Also we will probably add additional validation-related features, like this captureEvents modifier (aka "option 4"), which we might want to test with a particular @revalidateOn config. So should this then be in the validation file, the revalidate file or its own?

Idk, like I am not opposed to this, as I said I am just unsure how to draw the boundaries and not cause more confusion than we wanted to prevent by splitting this up...

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me, let's leave it as is


assert.true(
validateCallback.calledTwice,
'@validate is called for revalidation on blur'
Copy link
Contributor

Choose a reason for hiding this comment

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

oh I think I finally get how this revalidate is working!

as |form|
>
<form.field @name="firstName" as |field|>
<field.label>First Name</field.label>
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: are we going to test a multi-input field like the Date type we mentioned with day, month and year? That's the issue of what we were discussing last week right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great suggestion! Yes, we should do that, I'll keep that in my mind (hopefully, scold me when I forget 😉). But I would add that in a future PR that also adds the primitive to support that use case, which we currently don't have yet.

@@ -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!


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

Copy link
Contributor

@ynotdraw ynotdraw left a comment

Choose a reason for hiding this comment

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

Code looks great to me! I'm approving, but also curious about the isDestroy{ing,ed} comments above so going to track those.

@simonihmig simonihmig merged commit bc4750e into main Feb 2, 2023
@simonihmig simonihmig deleted the validation-timing2 branch February 2, 2023 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants