Form components: Support async validation #71184
Conversation
mirka
added
[Type] Enhancement
|
Size Change: +430 B (+0.02%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
Nice, I reckon this works quite well. Instead of an icon, do you think it would be feasible to show a small |
|
Something like this? CleanShot.2025-08-14.at.06-00-24.mp4I guess 12px is about the smallest we can go for this spinner? 
|
|
I added some logic so the |
|
LGTM! :) |
| // TODO: Technically, we could add an optional `customValidity` string prop so the consumer can set | ||
| // an error message at any point in time. We should wait until we have a use case though. | ||
| customValidator?: ( currentValue: V ) => string | void; | ||
| customValidityMessage?: { |
There was a problem hiding this comment.
I don't love this customValidityMessage name, because it muddles the actual customValidity concept in HTML with our own validating and valid statuses.
But given that most consumers will only be dealing with the invalid state that does indeed manipulate HTML customValidity, I think this is the way to go 😕
| const [ errorMessage, setErrorMessage ] = useState< string | undefined >(); | ||
| const [ statusMessage, setStatusMessage ] = useState< | ||
| | { | ||
| type: 'validating' | 'valid'; | ||
| message?: string; | ||
| } | ||
| | undefined | ||
| >(); |
There was a problem hiding this comment.
I considered merging the errorMessage and statusMessage into a single status state, but it ended up being more lines of code and not necessarily easier to reason about 😕
I think this is because statusMessage is arbitrarily set by the consumer, while errorMessage is dictated by HTML-level validity data and should always be prioritized over statusMessage. It's kind of easier to track as separate things, and only deal with prioritization at display time (i.e. when it hits ValidationIndicator).
| // Wait before showing a validating state. | ||
| const timer = setTimeout( () => { | ||
| setStatusMessage( { | ||
| type: 'validating', | ||
| message: customValidityMessage.message, | ||
| } ); | ||
| }, 1000 ); |
There was a problem hiding this comment.
I also considered handling this at the CSS level, but it turned out that doing it with timers was cleaner.
| setErrorMessage( validityTarget?.validationMessage ); | ||
|
|
||
| setStatusMessage( undefined ); | ||
| return undefined; |
There was a problem hiding this comment.
This is a weird case where our repo rules conflict. Without a return statement, TS emits a "Not all code paths return a value" warning. If I just do return without an explicit undefined value, eslint wants to remove it for being an unnecessary return statement 😅 Hence the return undefined which looks quite meaningless here.
There was a problem hiding this comment.
I was able to fix this by explicitly declaring the return type of the effect callback:
useEffect( (): ReturnType< React.EffectCallback > => {Ugly but works. For some reason TypeScript infers that the return type is undefined | () => void instead of void | () => void. The first one requires return statements.
There was a problem hiding this comment.
I will try this, thank you!
There was a problem hiding this comment.
I've been punting the task of adding tests for these behaviors, since this was kind of temporary and everything is still in flux. But I probably should do it soon (in separate PR).
| /** | ||
| * To provide feedback from server-side validation, the `customValidityMessage` prop can be used | ||
| * to show additional status indicators while waiting for the server response, | ||
| * and after the response is received. | ||
| * | ||
| * These indicators are intended for asynchronous validation calls that may take more than 1 second to complete. | ||
| * They may be unnecessary when responses are generally quick. | ||
| */ | ||
| export const AsyncValidation: StoryObj< typeof ValidatedInputControl > = { |
There was a problem hiding this comment.
@oandregal I don't know how much we want to abstract this in the DataForm APIs, but at the component level I tried to keep it flexible for now. If real-world use cases start to show patterns, maybe we can abstract it a bit more at the component level as well.
The biggest unknowns for me right now are:
- What percentage of use cases will need async?
- What percentage of those are slow enough to need a
validatingstate?
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Nice! As an aside, I wonder if we should turn the original icon in the first video into an animation instead of the spinner. |
| <ValidationIndicator | ||
| type="invalid" | ||
| message={ errorMessage } | ||
| /> | ||
| ) } | ||
| { ! errorMessage && statusMessage && ( | ||
| <ValidationIndicator | ||
| type={ statusMessage.type } | ||
| message={ statusMessage.message } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
I wonder if the level of customizability of the ValidationIndicator is enough. I somehow expected that the consumer would be able to provide their own ValidationIndicator component if they wish. I think the current spinner or icon + message indicator might be a bit limiting.
There was a problem hiding this comment.
I'm hoping we can keep it standardized here, given how we've been trying to hard line on the standard icons/color in regard to the intent in Badge. The message part is also prone to misuse, similar to labels and aria descriptions, in the sense that it shouldn't contain semantic content like links.
That said, my longer term plan as part of #71196 is to make all the these low-level components available as modular pieces that can be used to build any custom validated control. So when that time comes, yes, I think we can provide avenues for more customization in this area. By that time we should have a better idea of the customization needs as well.
| const ICON = { | ||
| valid: published, | ||
| invalid: error, | ||
| }; |
There was a problem hiding this comment.
That object will never change so it can be declared outside of the component.
There was a problem hiding this comment.
I thought about leaving this review comment as well, but half-paused on the idea in considering whether it's better to maybe allocate an object on each render vs. always allocating an object for a component that might never be rendered 🤔
packages/components/src/validated-form-controls/validation-indicator.tsx
Outdated
Show resolved
Hide resolved
| setCustomValidityMessage( { | ||
| type: 'invalid', | ||
| message: 'This checkbox may not be checked.', | ||
| } ); |
There was a problem hiding this comment.
The naming feels a bit off - we're "setting a validity message" but it's an object which also includes "message" and "type". Should we just call it "validity" instead of "validity message"?
There was a problem hiding this comment.
That makes a lot of sense, plus it's much shorter! 🙈 Will do that.
packages/components/src/validated-form-controls/components/stories/overview.story.tsx
Outdated
Show resolved
Hide resolved
| onValidate={ ( newValue: any ) => { | ||
| const message = field.isValid?.custom?.( | ||
| { | ||
| ...data, | ||
| [ id ]: newValue, | ||
| }, | ||
| field | ||
| ); | ||
|
|
||
| if ( message ) { | ||
| setCustomValidityMessage( { | ||
| type: 'invalid', | ||
| message, | ||
| } ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This part is very repetitive with the other field types, I wonder if we should abstract.
There was a problem hiding this comment.
I agree. I think some abstraction will be inevitable especially when the async support is added in here. TBD! (cc @oandregal)
| if ( ! customValidityMessage?.type ) { | ||
| validityTarget?.setCustomValidity( '' ); | ||
| setErrorMessage( validityTarget?.validationMessage ); | ||
| setStatusMessage( undefined ); | ||
| return; | ||
| } | ||
|
|
||
| switch ( customValidityMessage?.type ) { |
There was a problem hiding this comment.
The first check is really considering whether customValidityMessage is undefined, right? Since type is expected to always exist if it's defined.
Could we simplify some of these checks to assume if it gets past the first condition, customValidityMessage must be truthy.
| if ( ! customValidityMessage?.type ) { | |
| validityTarget?.setCustomValidity( '' ); | |
| setErrorMessage( validityTarget?.validationMessage ); | |
| setStatusMessage( undefined ); | |
| return; | |
| } | |
| switch ( customValidityMessage?.type ) { | |
| if ( ! customValidityMessage ) { | |
| validityTarget?.setCustomValidity( '' ); | |
| setErrorMessage( validityTarget?.validationMessage ); | |
| setStatusMessage( undefined ); | |
| return; | |
| } | |
| switch ( customValidityMessage.type ) { |
There was a problem hiding this comment.
The reason I wanted to avoid referencing the customValidityMessage object directly in this useEffect is so it doesn't need to rerun on object reference changes. But I guess is doesn't matter because getValidityTarget is inlined anyway. At the very least we can remove the second conditional though.
| switch ( customValidityMessage?.type ) { | ||
| case 'validating': { | ||
| // Wait before showing a validating state. | ||
| const timer = setTimeout( () => { |
There was a problem hiding this comment.
I wonder if this should be using some sort of shared ref for the timer and assigning as timerRef.current ??= ... ? If somehow this useEffect were triggered without any meaningful changes (e.g. new getValidityTarget reference because it's defined as an inline function) then the timer could reset unnecessarily.
There was a problem hiding this comment.
I'm not sure how that would work, since the clean up function would always have to clear the timer before the next useEffect run anyway. I may be failing to think of certain edge cases, but in the general case this effect will rerun on every user input value change (due to the unmemoized getValidityTarget), and I think it should indeed keep resetting the timer until the user input value settles down.
In any case, there's a good chance we'll need to tweak these micro-interactions if they don't feel quite right in practice. These are controlled demos, and I still found it tricky 😕
| } | ||
|
|
||
| return null; | ||
| setCustomValidityMessage( undefined ); |
There was a problem hiding this comment.
I wondered if allowing onValidate to return a customValidityMessage object would be a simpler way than requiring the component to maintain their own state, but I guess it's valuable to allow someone to pass customValidityMessage outside just this callback, and trying to manage the possibility of both onValidate return values and a direct customValidityMessage prop would be confusing?
There was a problem hiding this comment.
Right. I do miss the simplicity of returning a status from onValidate.
The original exploration by André with async callbacks can technically work if we didn't have to customize the "pending" messages, but still a React UI component accepting a Promise prop is a bit strange to me. Basically any UI component data could be fetched async, after all, and the expectation is that the consumer will handle it.
resolve/reject style APIs are also kind of roundabout:
onValidate={ ( value, validating, valid, invalid ) => {
validating();
fetchValidationResult( value )
.then( ok => ok ? valid() : invalid() );
} }onValidate={ ( value, setStatus ) => {
setStatus( 'validating' );
fetchValidationResult( value )
.then( ok => ok ? setStatus( 'valid' ) : setStatus( 'invalid' ) );
} }So all in all I feel this is the most straightforward and idiomatic, at least at this level.
There was a problem hiding this comment.
Async generators could be another option, but maybe a bit too unfamiliar for most folks to be ergonomic.
onValidate={ async function* () {
yield { type: 'validating', message: 'Validating...' }
const ok = await fetchValidationResult( value );
if ( ok ) {
yield { type: 'valid', message: 'Validated' };
} else {
yield { type: 'invalid', message: 'Error' };
}
} }There was a problem hiding this comment.
From experience with using async generators in custom data stores, I can tell folks found them a bit unintuitive, complex, and hard to read.
There was a problem hiding this comment.
onValidate could return an async iterator, that's exactly the semantics we're looking for here. An async stream of values (statuses) that ends after some time.
The caller could then read the validation progress in a for await loop:
for await ( const validity of onValidate() ) {
setCustomValidity( validity );
}Andrew's example async generator returns exactly this kind of iterator.
From experience with using async generators in custom data stores, I can tell folks found them a bit unintuitive, complex, and hard to read.
There is a big difference between data controls and async iterators. In data controls, the generators are used to implement a custom domain specific language, where the yielded objects have special meaning interpreted by a special runtime (redux-routine). Very custom stuff, quirky and poorly documented. On the other hand, async iterators are a core language feature. Should be much easier to get familiar with them. And LLMs should be perfectly fluent in understanding and writing them.
There was a problem hiding this comment.
it's valuable to allow someone to pass
customValidityMessageoutside just this callback, and trying to manage the possibility of bothonValidatereturn values and a directcustomValidityMessageprop would be confusing
Going back to this part of what Andrew said — handling everything through the onValidate doesn't work well with hook-based data fetching (SWR, TanStack, etc.) for example, so we'll want a way to set the validity state outside of this callback anyway. We can revisit when we have a better idea of how common the async use cases are, and what patterns are used for fetching. Maybe we can introduce an abstraction at a different layer.
Same linear speed as CleanShot.2025-08-16.at.05-08-29.mp4With a bit more snap: CleanShot.2025-08-16.at.05-07-25.mp4Or something else? |
An icon is not enough to communicate the status
|
That looks a bit clunky to me. I think it would work better if the filled section resized, as if each of the straight edges were like hands on a clock face, but that seems like a lot of work. We have a dedicated component for this ( |
This reverts commit 88ab560.
# Conflicts: # packages/components/CHANGELOG.md
|
Reverted back to the |
|
Thanks, I've updated the Dataform API at #71161 to leverage this. |
Prerequisite for #71161
What?
Adds support for async validation in the validated form components.
This is a breaking change for these private components.
For the scope of this PR, DataForm is only updated to maintain parity with the existing features, i.e. synchronous validation only.
Why?
Sometimes we'll want to show validation results from the sever.
How?
Separates the
customValidatorprop into anonValidatecallback and acustomValidityMessageobject. The intent is for this to be flexible enough to support different data fetching systems.Testing Instructions
npm run storybook:devand see the stories for the Validated Form Controls.The async story is on the Overview page.
Code review
It looks like a lot, but the bulk of the code lines are just simple changes to update existing usages to the new
ControlWithErrorcomponent API. The important changes are in the following three files:overview.story.tsx— An example of how async validation could be implemented.Screenshots or screencast
CleanShot.2025-08-13.at.14-03-43.mp4