-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add submit validation errors #35
Conversation
Oh and I didn't want to keep appending to the app.component.ts so I added routing to the demo app. |
b70ff10
to
377de2c
Compare
@@ -0,0 +1,26 @@ | |||
# https://github.com/numtide/devshell |
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.
this file shouldn't have ended up in here, I made a seperate branch for it, but rebase go burr i guess.
I can't develop without it as it sets up node 16 (which is no longer maintained btw, so nixos considers it insecure).
flake.nix can't, and shouldn't be, git ignored. Having to remake and remove this file every time I need to work on enhancy forms is really irritating, so it would be very helpful for me if this is in version control.
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.
@olistrik reviewed
} catch (e) { | ||
if (e === invalidFieldsSymbol) { | ||
return; |
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 prefer to leave the comment explaining why the error was swallowed. What is the reason for removing that?
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 also prefer those comments! I must have removed it by mistake.
_path: string; | ||
_message: 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.
- missing public/private.
- What is the reason for starting with
_
?
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.
they should have both been private, that's true.
the underscore is because they clash with the public getter otherwise.
private _foo: string;
public get foo() {
return this._foo;
}
is a common js encapsulation style. though some people prefer using $
instead of _
but I read that as sfoo.
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.
but I've removed them in my new changes.
|
||
constructor(path: string, message: string) { | ||
super(message); | ||
this.name = 'FORM_VALIDATION_ERROR'; |
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.
since the name is always FORM_VALIDATION_ERROR
, can just be a static final constant?
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.
As in a constant in the file? name
is defined by the error interface, I can't make it static readonly
in the class as that conflicts with the definition in the super class.
get path() { | ||
return this._path; | ||
} |
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 consistency's sake, getPath()
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 could also make it like this?
class ... {
public readonly path: string;
private message: 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.
actually, this would be sufficient:
export class FormValidationError extends Error {
public readonly name = 'FormValidationError';
public readonly path: string;
constructor(path: string, message: string) {
super(message);
this.path = path;
}
}
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 could also have name reference this.constructor.name
. That should be static afaik.
export const KLP_FORM_ERROR_HANDLER = new InjectionToken<FormErrorHandler>('KLP_FORM_ERROR_HANDLER'); | ||
|
||
export const DefaultErrorHandler: FormErrorHandler = (error: any) => { | ||
if (Array.isArray(error) && error.reduce((acc, err) => acc && err instanceof FormValidationError)) { |
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.
A reduce inside the condition is kinda hard to read. Also, this reducer does not have an initial value?
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 move it outside the if statement.
From developer.mozilla.org:
If initialValue is not specified, accumulator is initialized to the first value in the array, and callbackFn starts executing with the second value in the array as currentValue.
which clearly doesn't make sense in this situation. So it'll need to be init'd.
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 added a utility for array typing.
|
||
@HostBinding('class._fullWidth') get _() { | ||
return this.fullWidth; | ||
private renderError = (e: FormValidationError) => { |
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.
Misleading name, this doesnt render anything, but it sets the error object on a formControl
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.
fixed!
832c97d
to
ccfb854
Compare
@jerbob92 @wouter-willems
This is what Jeroen and I discussed. I've added an error type that can be thrown from the onSubmit callback and will register message errors on the form fields based on it's path in the form. If you thow a "FormValidationError", or even an array of them, it'll work out the box. Any other error thrown will bubble up as before.
The default error handler function can be overwritten with a provider to allow for automatic, application wide error mapping. This is particularly useful for DocHorizon, as all of our validation errors from the backend follow the same schema, and 9/10 times map directly to the form structure.