feat(lib): formGroup validator oneOf

[maxime1992]

This closes #91

[zakhenry]

I think that really just means we should extract the types to their own file.

[maxime1992]

The point was to get access to the form types without having to specify them when calling the validator but I hear your point... (CF next answer)

[zakhenry]

While I understand this pattern is following the angular pattern of namespace.validatorname, I think that concept by angular is actually a bad idea, because it is unfriendly to tree shakers - if you use one validator, youre basically guaranteed to have to import them all. I think a simple pure function per validator, in it's own file is the most tree shaking friendly option

[maxime1992]

I started exactly how you described it.
But then realized we'd have to pass the types as argument =/
Not impossible... not really awful but slightly more heavy 🤷‍♂️?

Although, I do have to admit that it feels wrong to have validators defined within the class and require the class to be instantiated before being able to use them.

[maxime1992]

Making another pass on this one, do we really care about tree shaking here? I wouldn't expect ngx-sub-form to embed plenty of validators (if so they should really be extracted into a separated package IMO). Plus, I'm not sure that using a namespace is better for tree shaking 🤔 but yes could have functions where we need to pass the type to.

[zakhenry]

I might be missing some complex requirement here, but this feels like it could be simplified to the following (no idea if this compiles ;) )

const notNullKeys: Array<keyof FormInterface> = keysArray.filter(() => {
  const control: AbstractControl | null = formGroup.get(key as string);

  if (!control) {
    throw new OneOfValidatorUnknownFieldError(key as string);
  }

  return !isNullOrUndefined(control.value);
});

if (notNullKeys === 1) {
  return null;
}

return { oneOf: notNullKeys };

[maxime1992]

That code doesn't handle if multiple values are defined. If there's more than 1 value, which one to pick? That should just be an error IMO

[zakhenry]

why is this an array of arrays?

[maxime1992]

Because one form component may have multiple properties that are polymorphic.

Example:

interface User {
  id: string;
  vehicle: MotorBike | Car | Truck;
  animal: Cat | Dog;
}

In the case we want to handle them without breaking down into sub components we can do:

oneOf([
  ['motorBikeVehicle', 'CarVehicle', 'TruckVehicle'],
  ['CatVehicle', 'DogVehicle']
])

[zakhenry]

@maxime1992 I think I've understood now what you were working to achieve but I realised that my thinking was too complex to explain in a MR comment, so instead I wrote it down as a commit :) See #95

I'm not saying this is what we should do, just noting down an alternative idea for discussion. If we do decide my proposal is a better strategy I'll be happy to finish it up properly

[maxime1992]

typo

[maxime1992]

@maxime1992 I think I've understood now what you were working to achieve but I realised that my thinking was too complex to explain in a MR comment, so instead I wrote it down as a commit :) See #95

I'm not saying this is what we should do, just noting down an alternative idea for discussion. If we do decide my proposal is a better strategy I'll be happy to finish it up properly

@zakhenry I re-read my MR and yours, I still think that managing the case where you have multiple oneOf on the same form is a good idea and could come up for ex a Person which has cat|dog and flat | house and car | motorBike. In our case at cloudnc a ToolAssembly which has a OneTool and a OneHolder.

Let me know what you think :)