FormGroup and ControlConfig refactor

[ayoayco]

Hi, everyone!

I'm thinking of changing the argument we pass to the FormGroup constructor a bit.

As you probably noticed, the name is the only required property, even the type is optional as we default this to text. This is not accidental, we patterned this from attributes that the out-of-the box HTML element <input> takes.

Now, consider the current configuration in our example:

const form = new FormGroup([
  {
    name: "username",
    label: "Username",
    validators: [Validators.required]
  },
  {
    name: "password",
    label: "Password",
    type: "password",
    validators: [Validator.required, Validators.minLength(8)]
  },
]);

It takes an array of ControlConfig objects.

I'm thinking of changing this so that the FormGroup constructor will instead take one object:

const form = new FormGroup({
  username: {
    label: "Username",
    validators: [Validators.required]
  },
  password: {
    label: "Password",
    type: "password",
    validators: [Validator.required, Validators.minLength(8)]
  },
});

I think this better represents the configuration we want, makes it simpler for users, and will have more flexibility later on.

Let me know if you see some possible problems with this!

cc team members @fazzaamiarso @Icyscools @Preet-Sojitra @Lalit3716

[Lalit3716]

Hmm I think it will be a good quality update for users, there shouldn't be any problems with this as well. It's a nice idea we should do this 👍

[Icyscools]

Interesting but I think it will make us refactor a lot of our code since the current FormGroup are treated as a list of FormControl. 🤣

[fazzaamiarso]

It definitely is a more viable and familiar approach. Manipulating types with Object also easier. I support this, but as @Icyscools said it will cost us some time to refactor.

[Icyscools]

I just thought of a problem that can cause if we redesign the FormGroup constructor in that way.

Normally, we store the FormControl in a list that was ordered collection. So, we can render the form controls in order like their order in a list.

In this purposed design, we will change a list to an object to store FormControls. I think it's good for a developer experience so that we can name the FormControl property named for a FormGroup. In fact, it'll reference easier and has more readability in the code, for example:

// with current design
let formUsername = form.get('username')
let formUsername = form.get('password')
// with the purposed design
let formUsername = form.username
let formPassword = form.password

However, as I told you: the FormControl are stored as a list and we're using its order to render. On the other hand, an object in JavaScript isn't guaranteed to order its properties. (see this). So, I don't think it's a good idea for now, we need to consider about it.

[ayoayco]

Regarding this...

// with the purposed design
let formUsername = form.username
let formPassword = form.password

I think we will still keep the .get('name') method, I don't know a way to dynamically add the control names as properties to the FormGroup class. Maybe via getter functions? But still I don't know how to have dynamic names for those... but thats a nice thing to explore.

[fazzaamiarso]

@ayoayco I believe we can do it with Proxy pattern with enforced types. By using proxy, we can also throw error if the properties user's trying to access doesn't exist.

[fazzaamiarso]

Ok, so let's prioritize that feature first. I might have some time to work on it in this weekend. 😄

@Icyscools let me know if you need another hand 😁

[ayoayco]

@ayoayco I believe we can do it with Proxy pattern with enforced types. By using proxy, we can also throw error if the properties user's trying to access doesn't exist.

Makes sense! I think this can even be worked on now even before this refactor, if I understand correctly

[fazzaamiarso]

@ayoayco that's right!