vaadin / vaadin-text-field

The themable Web Component providing input controls. Part of the Vaadin components.
https://vaadin.com/components
Apache License 2.0
25 stars 23 forks source link

Custom validators #28

Closed tomivirkki closed 7 years ago

tomivirkki commented 7 years ago

Definition of Done: add examples how to extend the VaadinTextFieldElement to provide:

limonte commented 7 years ago

@jouni your help in designing this feature needed :)

There are 2 ways to implement "Custom validators" functionality:

  1. function-way
  2. promise-way (ES6-way)

Function-way:

Pros: straightforward, easy-to-use solution

Cons:

function customValidator(value) {
  return condition? true : false;
}

Promise-way:

Pros:

Cons: could be a little bit harder for users who are not familiar with ES6

function customValidator(value) {
  return new Promise(function (resolve, reject) {
    if (condition) {
      resolve()
    }
    reject('this message can be customized')
  })
}

Since vaadin-input will be Polymer2 and have ES6 support from the beginning, my strong opinion is to use promises. On the other hand, @platosha's opinion is in favor of functions.

@platosha please put arguments for the function-way in case I missed something.

@jouni which way is better in your opinion?

jouni commented 7 years ago

This part feels like a blocker to me:

can't be async (server-side validation)

The use case I have in mind is a “username” field, that needs to be checked on the server to see if that username is already taken.

I’m not sure if a validator is the only way to accomplish this, but it feels semantically correct at least to “validate” the availability of the username.

Saulis commented 7 years ago

can't be async (server-side validation)

I'm not sure if this is entirely accurate – the way I see it is that after the custom validator is run, the status of the component changes as result.

So in the first option is more like:

function validator(value, callback) {
  callback(condition);
}
jouni commented 7 years ago

Right. In general, I don’t have a strong preference for either. All I know is that Promises is supposed to be some sort of an answer to “callback hell”, so it feels like the more appropriate way.

I think we should support both “simple” and more complex validators. Meaning, that the user can return either a Boolean or a Promise from the validator. In the case of a Promise, we could expose a state for “validation-in-progress”, which could be used in styling (a spinner for example). Also, I think the successful return from a Promise (resolve) should be allowed to return a string message.

function validator(value, callback) {
  // Either return immediately, client-side validation
  return condition? true : new Error('custom error message');

  // or use a Promise to do server-side validation
  return new Promise(function (resolve, reject) {
    if (serverCondition) {
      resolve('custom success message');
    }
    reject('custom error message');
  })
}
platosha commented 7 years ago

@jouni the callback argument is not needed then, right?

limonte commented 7 years ago

Yup, supporting both ways isn't hard to implement. In that case we don't need the callback parameter.

platosha commented 7 years ago

I favour supporting only sync functions for validators in order to restrict the validation to be sync-only altogether.

Async server-side validation is a valid use case, but this allows for a bigger delay, while non-validated-yet value is already displayed to the user. And, while validation is happening, the displayed value is neither valid nor invalid. This introduces a new UI state for the component, validation-in-progress, apart from valid and invalid we already have.

Consider it this way. If we don’t support async validation in the element:

If we support async validation in the element:

I doubt that the DX improvement benefits are worth introducing a new UI state at this point. Async validation altogether does not feel like a critical feature, would like to postpone until future minor.

jouni commented 7 years ago

Right, forgot the callback argument there. Was first thinking about supporting all three options even :) (boolean, callback and promise)

Async validators can be postponed to a minor IMO also. Would be nice to have an example how to implement that with a value change listener, though, and one that triggers it before the user blurs the field (debounced).

platosha commented 7 years ago

One more thing: having validation sync-only makes it easy to prevent invalid input #27.

Having async validation, invalid input can not be prevented in the same way, since potentially invalid values will have to be displayed in the input while validation is in progress.

Saulis commented 7 years ago

We prototyped extending the <vaadin-text-field> with Anton and turned out it's already pretty simple to implement both async validation and custom validation with invalid input prevention: https://jsfiddle.net/Saulis/eqf2ph1c/ no API changes needed.