vaadin / web-components

A set of high-quality standards based web components for enterprise web applications. Part of Vaadin 20+
https://vaadin.com/docs/latest/components
429 stars 82 forks source link

[crud] add support for @hilla/form binder #3545

Open manolo opened 2 years ago

manolo commented 2 years ago

Describe your motivation

Since the java counter part of crud is already integrated with flow binders, the same should happen when using the component in a hilla project.

Describe the solution you'd like

Setting a binder to the crud should allow to use server side validations and required flag out-of-the-box, as well as automatic binding of the generated crud fields in the editor.

API could be like something like:

crud.binder =   new Binder(this, SamplePersonModel, {onSubmit: SamplePersonEndpoint.update});

NOTE: that if binder is not present the web component works as usual, and component should not depend on hilla, just use it when it's provided

sissbruecker commented 2 years ago

@rolfsmeds @yuriy-fix Please organize discussion with Hilla team.

web-padawan commented 2 years ago

Since there is now a PR for adding the Binder support #4031, let's discuss and agree on the acceptance criteria first.

It would be nice to avoid adding direct dependency on Hilla in the web components:

  1. None of the components currently have it, and I don't see why vaadin-crud should be different in that regard,
  2. Adding Hilla dependency would make these two projects tightly coupled, and increase releasing complexity.

Instead, I would propose to figure out which events we should add to the web component. Based on the PR, the logic implemented in the web component could look e.g. like this:

const evt = this.dispatchEvent(new CustomEvent('item-edit-started', { detail: { item }, cancelable: true }));
if (!evt.defaultPrevented) {
  // Initialize form...
}

There are already other events in the web component that follow this approach (e.g. save event). Once we have this implemented, it should be possible to setup a Binder instance from outside.

And then we would need another event to call binder.clear() that could be named e.g. item-reset.

manolo commented 2 years ago

Note that this PR does not depend on Hilla. The idea behind is

web-padawan commented 2 years ago

Note that this PR does not depend on Hilla.

There is an implicit dependency introduced by using Binder in type definitions, which currently makes TS check fail:

$ tsc
Error: packages/crud/src/vaadin-crud.d.ts(38[6](https://github.com/vaadin/web-components/runs/6830323928?check_suite_focus=true#step:7:7),11): error TS2304: Cannot find name 'Binder'.

Adding @hilla/form to "devDependencies" would fix our checks, but not the users apps if they use TypeScript.

Crud could have it's own binder interface, so that any implementation that follows that API (like Hilla) might work

This sounds like a valid approach, as soon as we are confident that internal Binder interface would be compatible with the one provided by Hilla, so that any potential change in the @hilla/form Binder would not result in breaking the component.

If we agree to follow this approach, we need to implement the Binder API with corresponding unit tests. Then we can add a real test using @hilla/form to the integration folder of this monorepo.

manolo commented 2 years ago

There is an implicit dependency introduced by using Binder

Yeah, the default implementation in the prototype was not added by accident, added now

we need to implement the Binder API with corresponding unit tests.

Exactly, this is a one hour proof of concepts to see an implementation that really works, tests and proper interface is needed.

Defining an API entry for registering fields to the binder, might be also necessary, I mean when the default form implementation with dynamic fields is used, Hilla does not have the option to programatically add fields though.