Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using form parent properties in transformFrom/To #286

Closed
lgaida opened this issue Dec 6, 2022 · 5 comments
Closed

Using form parent properties in transformFrom/To #286

lgaida opened this issue Dec 6, 2022 · 5 comments
Labels
flag: can be closed? This issue or PR should probably be closed now

Comments

@lgaida
Copy link

lgaida commented Dec 6, 2022

Hi there,

i just migrated my application from the old to the new api. A few forms behaved differently after the migration but i found the issue quickly. I know that this is not really a bug but i still want to share it and maybe spark a discussion.

angular@13
ngx-sub-form@5.3.2

With the old inheritance-based api i was able to pass additional data via @Input() into my component and since my component extended NgxAutomaticRoot/NgxRoot/NgxSubFormComponent i could use this data inside the transformToFormGroup and transformFromFormGroup functions.

@Input('configurations')
public configurations: Configuration[] = [];
...

protected transformToFormGroup(client: Client | null,  defaultValues: Partial<ClientForm> | null): ClientForm | null {
  if (!client) {
    return null;
  }
  const configuration =  this.configurations.find((configuration) => configuration.id === client.configurationId) ?? null;
  return {
    name: client.name,
    configuration: configuration,
  };
}

So after migrating to the new api all the forms which had such additional @Input properties failed because this simply is in the wrong context when transformToFormGroup is called, thus this.configurations is undefined.
In order to fix this i had to wrap my transformToFormGroup inside a construction function which accepts and keeps backreference to my component. Think of it as a common var that = this; situation.

interface TransformFunc {
  (client: Client | null): ClientForm | null;
}
...
private backReferencedTransformToFormGroup = ( backreference: any ): TransformFunc => {
   return (client: Client | null): ClientForm | null => {
      if (!client) {
         return null;
      }
      const configuration = backreference.configurations.find((configuration) => configuration.id === client.configurationId ) ?? null;
      return {
        name: '',
        configuration: configuration,
      };
   };
};
...
public form = createForm<Client, ClientForm>(this, {
    ...
    toFormGroup: this.backReferencedTransformToFormGroup(this),
});

Check a full example at https://stackblitz.com/edit/angular-ivy-t8fvta

Unsurprisingly this is also the case for transformFromFormGroup and i can even think of a lot more cases where a backreference is needed since it is not an @Input()-dependent problem but applies to all cases where a transformFunction needs access to the actual component properties.

Depending on the use-case/complexity one could also do this kind of mapping/transformation in the parent-component and not the form-component. But i suppose that's why ngx-subform allows us to define a function for toFormGroup and fromFormGroup in the first place.

@lgaida
Copy link
Author

lgaida commented Dec 6, 2022

Here is another simple example

private transformFromFormGroup(formValue: Cost): Cost | null {
    if (this.anythingOtherThanCurrencySet(formValue)) {
      return formValue;
    }
    return null;
  }

this.anythingOtherThanCurrencySet fails since this in undefined.
Inlining anythingOtherThanCurrencySet is possible but degrades readability.

@maxime1992
Copy link
Contributor

Hi, thanks for the detailed bug report.

I know that this is not really a bug but i still want to share it and maybe spark a discussion

👍

That is indeed a breaking change that was not anticipated and not listed in the changes.

While it's (as far as I remember) not intentional to not have this in the same context, I think that's actually a good thing. You have no guarantee at all that your inputs will be set before these hooks are run (if the data is async). But whether it's async or not, there's a possibility of nasty bugs here where the input changes but it's not taken into account within your form (nor the output if it emits before the input is set).

In this particular case, if you want to manage the transformation from the form component I think it'd be more appropriate to use a combineLatest with your input transformed as an observable, your form value, and patch the form back.

@lgaida
Copy link
Author

lgaida commented Dec 6, 2022

Hi, thank you for your response :)

I do agree with you that the way i'm currently handling this opens some space for nasty bugs. In my case i'm displaying the actual form after the required data is loaded in the parent component, so i'm sure the data is there when the form is created. It worked fine until now so i'm okay with keeping it this way.

I've played around a little more and turns out that i was a bit too fast while migrating.
While migrating i just removed the extends Ngx* and tried to reuse the existing transform-functions like this

public form = createForm<Client, ClientForm>(this, {
    ...
    toFormGroup: this.transformToFormGroup,
    fromFormGroup: this.transformFromFormGroup,
});

and since the signatures matched everything looked fine.

However there is a contextual difference between the above and the following:

public form = createForm<Client, ClientForm>(this, {
    ...
    toFormGroup: (client: Client) => this.transformToFormGroup(client),
    fromFormGroup: (formValue: ClientForm) => this.transformFromFormGroup(formValue),
});

wiring it up this way keeps the correct this, function-calls and field-access now work like i initially anticipated.

@maxime1992
Copy link
Contributor

Oh yes, well spotted! I think it's now ok to close this issue as you've got your answer?

@maxime1992 maxime1992 added the flag: can be closed? This issue or PR should probably be closed now label Dec 7, 2022
@lgaida
Copy link
Author

lgaida commented Dec 7, 2022

Yes, closing this issue is totally fine. Thanks for reaching out again!

@lgaida lgaida closed this as completed Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flag: can be closed? This issue or PR should probably be closed now
Projects
None yet
Development

No branches or pull requests

2 participants