[client][form] feedback and minor issues

[vicb]

I discussed the form package with @haijian-vaadin on the demo repo and @vlukashov on twitter.

I have tried the framework in a Typescript project without Vaadin backend.
It's quite powerful. I really think it would benefit from being released as a separate npm package.
It would be easier to submit PR and write tests than by using this huge repo.

I have noted some potential minor issues while using the framework (at version 73ebb9eaf052afa2ad9843153a21b70eab67ee29)

Validator imports

Imports should be from es subfolder (i.e. import isEmail from 'validator/es/lib/isEmail';)

See the validator README for details

Missing return

I think there is a missing return in the Binder.

Shouldn't the code be

  async submit(): Promise<T|void>{
    if(this[_onSubmit]!==undefined){
      **return** this.submitTo(this[_onSubmit]);
    }
  }

Validation

The validation is triggered each time a blur event is received.
There would be no need to re-run the validation if the values (root binder) have not changed since the last validation.

It is not so much of a problem for simple sync validation.
However for more complex validation (i.e. checking if a username exists by querying the server) this means that a request is sent to the server each time the focus is changed.

Field strategy

Currently the only way to resolve to a different strategy is to subclass the binder and override getFieldStrategy()

It would be helpful to be able to override the field resolution in the BinderConfiguration:

export interface BinderConfiguration<T>{
  onChange?: (oldValue?: T) => void,
  onSubmit?: (value: T) => Promise<T|void>,
+ getFieldStrategy?: (elm: any) => FieldStrategy,
}

Probably the same could make sense for validators ?

Currently you should call binder.addValidator(...) to add a validator.
Having the option to add validators to the configuration would make sense.

[haijian-vaadin]

Hi @vicb, thanks a lot for your feedback. I made a PR for the first 2 easy fixes (Validator imports, Missing return). Others we will improve later.

[vicb]

Thanks a lot for the fixes @haijian-vaadin

I haven't taken to understand how unit tests work in this project.
Again I think that splitting this form framework in a separate vaadin module would make contribs much easier.

Do you have any plan to split it in a near future ?
If not, I will consider publishing a module. I think it's fine with the apache licence ?

Your form framework is great and much better than any other ones I looked at.
Also it's already pretty well documented. I actually only looked at the doc after I understood how it works,
but pretty much everything is well documented. The only missing doc would be how to create a model
class without the backed which is quite easy.

Thanks,
Vic

[knoobie]

@vicb you can find more about your publishing wish here #290

[vicb]

Thanks for the link @knoobie

[vicb]

There is a side effect of Validation: The validation is triggered each time a blur event is received (from my initial message).

Imagine a from with two field:

• field1 is validate on the client side,
• field2 is validated on the server side only.

Let's say field2 is invalid after you try to submit the form.
Now if you focus/unfocus field1, the validation is triggered and the error is removed from field2 (because it has no validator on the client side).

I am not sure validators that are only present on the server side are part of the initial use cases but anyway I think this can be fixed if validators are not re-run unless the values have changed (i.e. there would be no need to rerun the validator for field2 in this example).