search

cloudnc / ngx-sub-form

Utility library for breaking down an Angular form into multiple components
https://cloudnc.github.io/ngx-sub-form
MIT License
314 stars 33 forks source link

Major architecture changes coming #171

Closed maxime1992 closed 2 years ago

maxime1992 commented 4 years ago

Hello :wave:

What's this all about?

I'd like to share a little update on ngx-sub-form on behalf of @zakhenry and myself as we've been pretty quiet for a while. We've had some work at CloudNC we needed to focus on and with the Covid-19 coming in between, we've all had to adapt. So if you've commented on an issue, made a PR or made any kind of interaction and haven't had any answer don't take it badly. We still care a lot about ngx-sub-form and we'll try to get back to you asap :smile_cat:.

Better inclusion of ngx-sub-form into our planning (at CloudNC)

@zakhenry and I have been trying to build a dependency graph of all the issues, order and prioritize them in order to start including some of them as part of our work time (during our sprints :raised_hands:). We hope to tackle some tech debt on ngx-sub-form pretty soon :muscle:.

New architecture

I spent a whole day thinking about all the issues, how they connect to each others, how to solve one without adding unwanted behavior, etc. The following lead me to think that we need to rework on the architecture of the project and make major changes.

From what I've found, concepts will remain the sames but:

Changes

Let's start with a rough diagram of the new internal data flow. Please don't focus too much on this yet, I'll refer to it point by point explaining what's new and what it'll bring:

the image has a good quality, if it's not big enough open it in its own tab

image

Creating a sub/remap/root form component

First big change: Lets get rid of inheritance :+1:.

Here's how a basic sub form would look like with the new API:

@Component({
  // all the usual
})
export class SubFormComponent {
  public subForm: SubForm = createSubForm<YourSubFormType>(this, {
    formControls: {
      prop1: new FormControl(),
      prop2: new FormControl(),
    },
  });
}

Related issue:

Stop using internally { emitEvent: false } every time we interact with the public form group

Internally the biggest issue we face is how to apply changes to our form group when required by the parent (through writeValue or even setDisabledState for example) without broadcasting those values straight up to the parent.

When the value of the model changes upstream, we want to put it into the form group, wait for the user to update the form, and for every subsequent changes broadcast the value up. In order to achieve this, we previously used { emitEvent: false }:

https://github.com/cloudnc/ngx-sub-form/blob/00aea19134ba253a8244e1662f4fd985234449c2/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts#L108-L110

https://github.com/cloudnc/ngx-sub-form/blob/00aea19134ba253a8244e1662f4fd985234449c2/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts#L118-L119

https://github.com/cloudnc/ngx-sub-form/blob/00aea19134ba253a8244e1662f4fd985234449c2/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts#L245-L250

https://github.com/cloudnc/ngx-sub-form/blob/00aea19134ba253a8244e1662f4fd985234449c2/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts#L271-L273

https://github.com/cloudnc/ngx-sub-form/blob/00aea19134ba253a8244e1662f4fd985234449c2/projects/ngx-sub-form/src/lib/ngx-sub-form.component.ts#L422-L426

This way, the changes would be reflected on the form but our internal subscription to formGroup.valueChanges wouldn't be triggered straight away when writeValue is called.

Unfortunately, this led to some (annoying) issues:

How is the upgrade going to work?

We'll try to make the upgrade as smooth as possible by deprecating the old way of using ngx-sub-form through classes and expose the new functions once they're ready. This should let you do an incremental update if you've got a lot of components using ngx-sub-form. (and don't worry, we have ~120 components which extends Ngx*FormComponent so we'll have to go through that ourselves :sweat_smile:!)

We'll create a next branch were we'll be able to make pre-releases on CI so that we can thoroughly test internally that everything is fine before publishing it for good :+1: and ask for some help from the community to see if everything is fine for everyone!

The first target will be to reach features parity. Once we reach that objective we should be able to cut a release (a feature release, not a breaking change :ok_hand:). From there, already quite a few issues should be solved and we should be able to move on to new issues.

How can you help?

Please comment directly on this issue so that we can all end up with a good way of working with our forms :smiley:!

Following is just the Excalidraw backup for the schema ngx-sub-form-new-architecture.zip

ntziolis commented 4 years ago

First of ngx-sub-form has brought a massive boost in developer onboarding speed + productivity + code quality for us. Obviously I can only speak for my team, but the increases were so big that we wold be 100% fine with rewriting the hundreds of forms we have already written against the existing API, based on the advantages of the new approach.

I'm happy to participate in any way necessary (calls, implementation etc.) to get this off the ground quickly. My schedule is wide open for this (based in Europe).

Sidetrack: From what I understand the changes are mainly around how a sub form handles changes and when those changes are emitted and that's great. I'm assuming it would not address things like https://github.com/cloudnc/ngx-sub-form/issues/155 (and that's totally fine). I'm asking to understand if I should halt the work I'm doing to address those kind of issues.

ntziolis commented 4 years ago

This is going to be a long one, please bear with me. Since there is going to be some rearch I tried to think about what the benefits / goals are of ngx-sub-forms and what we (my team) have learned while using it. Then I reset my brain to try and see if there is a completely different approach, just to make sure we are not missing something. This is not meant to derail the main discussion and if you feel this issue is not the right place I'll open a new one.

First lets go though the key advantages of using ngx-sub-form:

At the core it achieves this by implementing ControlValueAccessor, which is an elegant solution that enable custom components to "just work" with the reactive form directives.

This approach solves A LOT of problems and has served us well, but it does have one major caveat. It hides the controls of a sub-form from the parent. However without standardization of the structure of sub forms, there was simply no other way to achieve enabling the above.

Maybe you already know where I'm heading with this. By using ngx-sub-form the structure of sub forms is actually well defined. Which does open an avenue outside of leveraging ControlValueAccessor.

The main three pieces would be:

Custom directive e.g. [subFromGroup]

"Wrappers" for FormGroup / ArrayForm

By making the wrappers generic it would even be possible to have typings for the controls on the sub-form (not just the output value). Granted, this means exposing the FormModel from the sub form to the outside world. So this should be optional.

Today there is a lot of smartness in the SubFormComponent base class especially around handling the values. This is due to the fact that it had to bride between ControlValueAccessor and the FormGroupon the sub form. By taking out the necessity to bridge the logic in the class would likely become significantly less complex.

The downsides of the non ControlvalueAccessor approach that I can think of right now are:

Enough words here is some code to illustrate.

Directive

// 99% copied from angular source
@Directive({
  selector: '[subFormGroup]',
  providers: [formDirectiveProvider],
  host: { '(submit)': 'onSubmit($event)', '(reset)': 'onReset()' },
  exportAs: 'ngForm'
})
export class SubFormGroupDirective<TControl, TForm> extends FormGroupDirective implements OnInit {
  @Input('subFormGroup') form: SubFormGroup<TControl, TForm>;

  constructor(
    @Optional() @Self() @Inject(NG_VALIDATORS) validators: Array<Validator | ValidatorFn>,
    @Optional() @Self() @Inject(NG_ASYNC_VALIDATORS) asyncValidators: Array<AsyncValidator | AsyncValidatorFn>,
    private host: NgxSubFormComponent<TControl, TForm>
  ) {
    super(validators, asyncValidators);    
  }

  ngOnInit(){
    // this method would be new on the sub form base class
    this.host.initFormGroup(this.form);

    // we need access to the sub form component instance to handle things like transformations
    this.form.attachSubFormComponent(this.host)
  }
}

Wrapper

export class SubFormGroup<TControl, TForm> extends FormGroup {
  private subForm: NgxSubFormComponent<TControl, TForm>;

  constructor(
    validatorOrOpts?: ValidatorFn | ValidatorFn[] | AbstractControlOptions | null,
    asyncValidator?: AsyncValidatorFn | AsyncValidatorFn[] | null
  ) {    
    // initialize an empty form group at first
    // controls will be added when form group is injected into sub-form
    super({}, validatorOrOpts, asyncValidator);
  }

  // required to allow injection of the sub form component from outside after the Wrapper was initialized
  attachSubFormComponent(subForm: NgxSubFormComponent<TControl, TForm>){
     this.subFrom = subFrom;
  }

  // override impl samples
  get valueChanges() {
    // here we use the injected sub form component instance to handle transformation
    return super.valueChanges.pipe(
        map(value => this.subForm.transformFromFormGroup(value))
    );
  }

  setValue(value: { [key: string]: any }, options: { onlySelf?: boolean; emitEvent?: boolean } = {}) {
    super.setValue(this.subForm.transformToFormGroup(value), options);
  }
}

I'd be super keen to know what you think. I understand this would be a complete departure from how the lib works today.

ntziolis commented 4 years ago

First off sry for spamming this thread, but couldn't let this rest yesterday and spend all night getting this up and running based on latest v4 (as we have a ng8 project where we need it). I did have to work through a bunch ts compiler issues, but once I worked through these data flow + advanced use cases like markAsTouched from parent worked pretty much right away.

This is with ControlValueAccessor and internal value changes handling completely ripped out of the SubFormComponent.

But (you knew it was coming):

At this point I thought this approach was a dead end and then it hit me.

Why not go even simpler:

With this ins mind, what would be the role of ngx-sub-form:

But it would not be involved in value / observable handling at all (besides transformation inside the wrappers for FormGroup / FormArray).

Thoughts? Totally understand if this is not a direction you would consider.

ntziolis commented 4 years ago

Couple of lines of code are better than thousand words (basics are already there and have it up and running in our project): https://github.com/ntziolis/ngx-sub-form/tree/sub-form-controls-simpler

maxime1992 commented 4 years ago

First lets go though the key advantages of using ngx-sub-form

Very good summary :+1:

This approach solves A LOT of problems and has served us well, but it does have one major caveat. It hides the controls of a sub-form from the parent. However without standardization of the structure of sub forms, there was simply no other way to achieve enabling the above.

Correct. Unfortunately ngx-sub-form was created initially because of the lack of support for sub forms.

Maybe you already know where I'm heading with this

I have a little idea :smile:!

The main three pieces would be: Defined structure of a sub-form (same / similar as today) A custom directive e.g. [subFromGroup] that replaces [formGroup] Wrappers for FormGroup / ArrayForm

I'll answer point by point:

Custom directive e.g. [subFromGroup]

This is a very interesting point. But I suspect we may run into some expression have changed error as modifying the sub form (which would be attached to the parent) would trigger instant changes for the parent component. Not 100% sure though...

"Wrappers" for FormGroup / ArrayForm

Instead they need to use new SubFromGroup() / new ArrayForm() when a property is representing a sub form

One gap I've been trying to close into https://github.com/cloudnc/ngx-sub-form/issues/147 is the fact that we're not sure at build time about how things get connected within the view. Unlike an input, there's no way to really type a control value accessor.... So we do not get any AoT safety. If we could guarantee that for sub forms that'd be :exploding_head:. I know it's not exactly what you were referring to but I got inspired ha.

Today there is a lot of smartness in the SubFormComponent base class especially around handling the values

As long as this smartness is black boxed into the lib and it's easy to consume I'm pretty happy with it. Also, with the ongoing rewrite I think I've managed to get a simpler workflow. Not saying it'll change much to the overall complexity but it should be way easier to read :smile:!

Custom directive instead of built in reactive forms directives

As you've probably noticed by now, we've tried to keep ngx-sub-form as a utility library. We've considered multiple times going all in and create a module that'd contain some directives, could have services etc. But we managed to avoid that (so far).

this.form.attachSubFormComponent(this.host)

As much as I like the idea, I think this would have quite a few issues with the expression has changed error.

I'd be super keen to know what you think. I understand this would be a complete departure from how the lib works today.

Some very inspiring ideas but I'm afraid some of them may not work properly in Angular world. But it might be definitely worth a try with at least a POC. The biggest thing holding me back though is the fact that sub forms could only work well together with sub forms from the lib. Imagine you want to consume from an external lib a sub form (which is implemented simply using ControlValueAccessor), that one wouldn't report up to the parent. But I hear you and understand the frustration around some edges with ngx-sub-form not being able to handle all the use cases for now.

First off sry for spamming this thread

Please don't apologize for that. You're raising really good points and I hope you keep doing that! :smile_cat:

ngOnInit on the SubFormComponent gets called prior to ngOnInit on the directive which injects the FormGroup instance into the SubFormComponent

This is no longer an issue with the rewrite I've been working on. I hope to be able to clean up a bit and share some code that you can look at soon, even though it's definitely not finished.

So far, I managed to:

What I couldn't get working well is the emitNullOnDestroy (which is annoying :sweat:) because of https://github.com/angular/angular/issues/36776 where basically it's not possible to override life cycle hooks on a component instance anymore :sob:. I really hope this PR https://github.com/angular/angular/pull/35464 gets merged soon :pray:!

This is a critical blocker as user should NOT have to place his init code in a custom method instead of ngOnInit which would really be the only way around this.

Agree. And I have a nice/safe way of hooking into a component instance to then expose all the life cycle hooks as observables :fire:... But as explained above it doesn't work with Ivy anymore...

Since a SubFormComponent is well defined, we could simply enforce a simple input property e.g. subForm which holds the SubFormGroup instance

I like that because I think we could get better type safety out of it!

its now @Input() subForm : SubFormGroup

How would that work?

Couple of lines of code are better than thousand words (basics are already there and have it up and running in our project): https://github.com/ntziolis/ngx-sub-form/tree/sub-form-controls-simpler

Wow never thought of that! That might actually be pretty nice yes but I'm unsure how you'd use that. Do you have any example consuming that new API?

ntziolis commented 4 years ago

First off, hats off to being open to input like that. I call this out as this has not always been my experience when providing feedback especially if it goes to the heart of a project.

I'd love to share the actual project I used while testing this, but as I was crazy enough to put it in one of our actual projects (to see where things blow up beyond simple usage). Hence, below is an outline of the key pieces.

If the parent is a root-sub-form (assuming the old structure for now):

// on the parent component
getFormControls(){
  return {
    // the key here is to use the "special" SubFormGroup class 
    // which understands that there is a sub-form below to be handled
    // no value can be supplied here, but validators / form opts can be provided
    mySubForm: new SubFormGroup() can be provided
  }  
}

// and in the parent template
<my-sub-form [subForm]="formGroupControls.mySubForm">

That's really is all you have todo to hook things up.

The ngOnInit method on the ngxsub-form base class is where the magic happens. It "injects" the sub-form component instance into the SubFormGroup to handle transformations

You raise a good point around interoperability with non ngx-sub-form parents. This would also have been a full stop blocker for us if not possible.

Here is what this would look like:

// on the parent component
this.form = fb.group({
   // syntax is really the same as for non sub forms
   mySubForm: new SubFormGroup()
})

// and in the parent template
<my-sub-form [subForm]="form.get("mySubForm")>

So yes, there is a dependency on usage of SubFormGroup:

Considering the above this feels like a good integration story that external devs can easily understand even if they have never seen ngx-sub-form.

However should this would not be acceptable, it would also be possible to create a generic class that wraps a sub-form into a ControlValueAccessor enabled component . It would be up to the dev of the lib to decide how he wants to expose the sub forms then.

Further usage:

ntziolis commented 4 years ago

I'm going to the rest of you comment and reply back later. Just wanted to make sure you have usage samples first.

ntziolis commented 4 years ago

Sry for the long silence. Work got in the way :) over the weekend I had time to migrate the demo project in the repo to use the outlined approach. Need to put some finishing touches on it and will report back here.

ntziolis commented 4 years ago

While building the below I kept in mind that we might wanne move away from inheritance. I do have some ideas how to make the approach below work without inheritance as well. I just based it on what was already available.

Status updates

The "good" thing no matter where this leads is I learned A LOT about reactive forms, not sure I wanted to know all that but I'm sure it will come in handy moving forward :)

ntziolis commented 4 years ago

So there seems to be some movement on forms https://www.twitch.tv/videos/620888602, trying to setup a call / twitch session

ntziolis commented 4 years ago

Very interesting read. I knew of mixings, but I didn't know about component features. Together with Feature interfaces its a great way to enforce structure of a component without inheritance: https://indepth.dev/component-features-with-angular-ivy/

maxime1992 commented 4 years ago

Hi @ntziolis :wave:

Brandon offered Zak and I on Twitter to talk about ngx-sub-form in his live stream :smile: We said we were interested but at least once this issue is closed. I think we can indeed push the architecture further and solve more issues but getting rid of inheritance won't be too much troubles for existing code base and will already solve quite a few annoying ones.

About the component features, we discussed it with @zakhenry and it could be a great match for us. Unfortunately this API is not public and I read comments on Twitter saying that the Angular team was not sure to open this up for public use as it gives them flexibility internally but exposing it would limit the changes they could make to it... So pretty unsafe to use as of today IMO.

I haven't taken a look to your demo yet but will do asap :)

ntziolis commented 4 years ago

Great to know Brandon reached out already, From the session so far I feel they are just barely scraping the surface. I don't think reactive forms in "simple" is bad at all. Sure they could be type safe + make it even easier to create single field sub forms.

But the main issues come up when forms get more complex and you wanne do proper multi field sub-forms. The other key missing piece is transformation which is especially important to us since we don't have control of the downstream systems (backend) in most of our projects.

To give some context we do a lot of "digitization" projects for enterprises. Usually this either means:

Its not uncommon for a single form to have 100+ fields. In addition in all cases the form validation requirements are very complex, here is an actual sample:

This would be a pretty standard use case we have to deal with (sadly im not kidding). Sometimes we have dependencies between 20+ controls on different forms to make a decision on a specific form control.

Given our project types our work is extremely forms heavy. I mean 90%+ of our work is building forms as auth, hosting, backend is handled by downstream systems and is out of our control. We have likely crossed the 2-3k forms mark by now for the last year alone.

Maybe with the above it becomes a bit more clear why I'm desperately trying to figure out if we could make sub-forming work without loosing access to the underlying controls.

I understand that our requirements are on the upper end of complex, but still hope that this could be made in a way that it doesn't add complexity for the simple use cases.

Obviously I'm happy to participate in a call / witch session if you feel it would be helpful. Just lmk.

hakimio commented 4 years ago

Its not uncommon for a single form to have 100+ fields. In addition in all cases the form validation requirements are very complex, here is an actual sample:

  • when the value of a form control on form A is set
  • and a specific control value on on form B has value x
  • and a specific form control on form C is not disabled

  • THEN

    • a specific form control on form D is required
    • and pre-filled with value from a control on Form E

You might want to take a look at Akita Forms Manager.

ntziolis commented 4 years ago

@hakimio Thank you for the suggestion (might be super helpful to others who find this thread). We already use it heavily and it cannot be stressed how super helpful it is :)

Sadly though there is no access to the individual form controls down the tree when using it together with ngx-sub-forms. The controls on sub-forms below the root form are hidden by ControlValueAccessor. Why does it matter:

The from outside the sub-form really is the key piece here and for better or worst is our default use case.

We could not use ngx-sub-forms, but we are in love with value transformations and the structure it imposes. Switching projects or onboarding new devs is a breeze (vs pain in the ass) since every form has the exact same structure. Hence the willingness to try out different paths to get the best of both worlds.

andreElrico commented 4 years ago

That being said, native strongly typed forms and ivy-features will probably not land very soon as framework changes progress slowly.

Just to add some feature request.

Im did not use ngx-sub-form lately, but I remember the countless times you helped me out @maxime1992 . Dont worry you two (+ @zakhenry ) about being inactive. Looking forward to ngx-subform2.

ntziolis commented 4 years ago

Was sadly to busy until recently. I was able to make major progress and now only have 2 issues left that I'm confident I'll be able to resolve it this "sprint".

Issue resolved:

What was changed:

Remaining issue are:

@maxime1992 @zakhenry You more than are welcome to review the currently pushed state. After I have resolved the last issues I'd love to have a chat with you to walk you through it in detail. You can then decide if this is something you wanne incorporate in to this lib or if I should separated it out in a new lib (my team has already played around with this extensively and it solves all the issues we were running into that I have outlined previously).

ntziolis commented 4 years ago

One down one to go:

This means:

I have also started to clean up a bit.

Will focus on getting formGroupErrors to work as it did before:

Le me know what you think on the error front.

maxime1992 commented 4 years ago

Hi @andreElrico :wave:

I think "get rid of inheritance" will be a big improvement. Thats the one thing I really disliked about this project

We're conscious this is not ideal but as form should have tiny scopes it wasn't acutally so bad IMO but still trying to come up with something better :)!

Also I've read that @angular is starting to work on strongly typed forms, so maybe this will make this lib even more lightweight

Yes, I'm following some issues too and I've seen some comment from Kara :). Although types are not the biggest part of this tiny code base, I'd be glad to remove them if there's a good native support :raised_hands:

That being said, native strongly typed forms and ivy-features will probably not land very soon as framework changes progress slowly.

Yes I doubt we're going to see that landing in the framework any time soon but :crossed_fingers:

It would be cool if polymorphic forms would be able to realize with ng-templates there is templateGuards to maybe this could be used to add typesavety.

Not sure to understand what you mean here?
Are you talking about something like type safe switch case in TypeScript? If so I've created an issue in July 2018 for that: https://github.com/angular/angular/issues/24978 Feel free to give it a :+1:

Also the polymorphic forms api was always quite challenging for me. Maybe there an easier way :/ .

Could you be more precise?

Im did not use ngx-sub-form lately, but I remember the countless times you helped me out @maxime1992 . Dont worry you two (+ @zakhenry ) about being inactive. Looking forward to ngx-subform2.

:smile_cat: :pray:

Hey @ntziolis :wave:,

thanks for putting so much effort into all this :fire:

Was sadly to busy until recently. I was able to make major progress and now only have 2 issues left that I'm confident I'll be able to resolve it this "sprint"

Us too. We've had 1 or 2 sprint in which we managed to save a lot of bandwidth for ngx sub form rewrite but then had to move on before I got done with it.

About the rewrite, I got pretty far and most of the e2e test are working but to get the errors correctly working without having an expression has changed error is a nightmare (as you seem to be aware ha). If you want to have a look into the progress and the rewrite you can here: https://github.com/maxime1992/ngx-sub-form/tree/feat/rewrite :+1:

Various ExpressionChanged errors (this was a blocker issue and by far the most complex of the remaining)

I feel your pain. This is one of the key point of ngx sub form. Deal with those nested structures that can easily end up with expression has changed errors so that people don't have to deal with them in their code base :fire: :smile: :muscle:!

It became clear that there is no way to make this work without access to ChangeDetectorRef

The library is using ng9, do you think the new (but private :disappointed:) function markDirty may be enough? If so I think we could use that and if it ever gets remove then ask people to provide the ChangeDetectorRef... But in the meantime it'll still mean that we don't need to provide anything and add any boilerplate :)

After I have resolved the last issues I'd love to have a chat with you to walk you through it in detail

Yeah I'd be happy to hear more about what you built as I've not been able to find some free time to dig into it yet! Don't know when we can do that but I've created a Discord server for ngx-sub-form so that we can easily jump into a visio conf when we feel like it :) Feel free to join here and we'll try to figure out a good time to chat: https://discord.gg/wNFg7ab

I have also started to clean up a bit. Will focus on getting formGroupErrors done, hopefully by eod tmr

Good luck!

maxime1992 commented 4 years ago

Hello :sun_with_face:

@zakhenry and I found some time during the weekend to continue a bit on the rewrite and I believe we reached a state where we can raise a public PR and let anyone have a look to the implementation and the new pubilc API :tada:.

The PR is here: https://github.com/cloudnc/ngx-sub-form/pull/176

We've duplicated our demo app to have both the old one and the new one (same but using the new API) and our E2E tests have been updated too to run on both apps:

image

It looks pretty promising but maybe some cases are not handled by our E2E tests so we'll need a solid help for manual testing and everyone can help by giving a go at the new version :raised_hands:.

Few things are still breaking changes at the moment but we'll do our best to make sure we have the old and the new version coexisting for a little bit so that people can have a smooth upgrade instead of a painful, "big bang" one.

As I said, still a WIP but we'd love to have some feedback onto it!

This wouldn't solve all the issues... But would still solve the following ones:
(appart from the oneOf which is unrelated here)

ngx-sub-form-issues-solved-after-rewrite

Currently the only way to give it a go is by cloning the repo, checking out the rewrite branch and building it. We understand it is pretty painful and we'll try to have a pre-release setup on that branch to make this easier for everyone who wants to help testing it.

:warning: Please don't rewrite all your forms yet :warning:
This is still a WIP and the public API may change if we discover major issues.

The best way to start would be to have a look on the PR https://github.com/cloudnc/ngx-sub-form/pull/176 and share your thoughts then look into the PR within our demo code to see how to use the new API. You could then give it a go on your side on some forms :+1:

Let us know what think, please don't be shy :smile_cat: !

evgeniyefimov commented 4 years ago

Any ETA for these changes?

maxime1992 commented 4 years ago

ETA:

I'm not :100:% sure of that yet but I think that as soon as we've got some feedback and if the API looks good, we may merge into master and just provide 2 different paths to import from. The regular one and maybe a new one like ngx-sub-form/new-api or something similar. This way we could incrementally improve this instead of having a long lived branch which may become an issue.

maxime1992 commented 4 years ago

Update: I've spent some time this week end to:

The split is done :heavy_check_mark: but unfortunately I hit an error when I try to make a pre release.
I've open an issue here https://github.com/semantic-release/github/issues/285 and it may be related to the name of the branch so hopefully I can solve that soon :+1:

maxime1992 commented 4 years ago

:tada: This issue has been resolved in version 5.2.0-feat-rewrite.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

maxime1992 commented 4 years ago

Wouhouuu it worked! :tada:

If anyone wants to try this out, ngx-sub-form@5.2.0-feat-rewrite.1 is available!

Haven't tried it myself yet as it's a bit late but... :grimacing: :crossed_fingers:

I'm going to close https://github.com/cloudnc/ngx-sub-form/pull/176 in favor of https://github.com/cloudnc/ngx-sub-form/pull/188 which is the same of except that I renamed the branch to go around the publishing error with the not supported format for the branch feat/rewrite which I renamed in the new branch to feat-rewrite.

maxime1992 commented 3 years ago

Hello :wave:!

:loudspeaker: We've got some important updates to share! :loudspeaker: :smile:

Some work has been done over the weekend on the refactor to make sure everything is working fine.
To do that, what's better than a demo app? A real world app!

So we've upgraded our ngx-sub-form version in our monorepo to v5.2.0-feat-rewrite.6:exclamation:
As we can use ngx-sub-form and ngx-sub-form/new this didn't bring any breaking change :fire:.

As the update can be incremental, we haven't committed yet to transform 100% of our forms. BUT. We did update a part of our app that contains a lot of root and sub forms. The diff before/after when searching for from 'ngx-sub-form' is 131/79 :smile_cat:!

The idea behind giving this an early try is to:

Guess what? We're glad we did it because we caught quite a lot of issues, missing features and we learnt a lot about the process of upgrading an existing code base! Our internal E2E tests on our app got us covered and caught a lot of regressions which we were able to identify and fix one by one (note the bump from v5.2.0-feat-rewrite.1 to v5.2.0-feat-rewrite.6 in 2 days :joy:).

But the good news is: Our internal codebase is now running on v5.2.0-feat-rewrite.6 and should go in prod pretty soon :eyes:.

What's coming next?

As things are looking pretty good but not ready to be released yet, I'll try to draft up a guide that'll reuse later on in the release note on how to migrate so that if anyone from the community feels brave enough to give a go to the new version, we'd love to hear some feedback onto it :smile_cat:!

I don't think the API on the new part is going to change a lot but keep in mind that those are just pre releases and we may modify slightly the API so if you're willing to maybe give it a go on a small part of your code base but if you're using ngx-sub-form a lot don't update everything just yet.

We'll be in touch soon :wave:

maxime1992 commented 3 years ago

:loud_sound: NEED INFO :loud_sound:

Who's using handleEmissionRate in his code base?

We do expose that method so that automatic root forms can be debounced for example:

protected handleEmissionRate(): (obs$: Observable<FormInterface>) => Observable<FormInterface> {
  return NGX_SUB_FORM_HANDLE_VALUE_CHANGES_RATE_STRATEGIES.debounce(300);
}

But this method is exposing the final observable (that is going to be sent to the parent through the Output) and this is something odd. Surely we shouldn't be exposing the form value in that hook. I needed it pretty quickly so in the new code base I did reimplement the same thing (as an object of the configuration now):

public form = createForm<ToolHolderFilter>(this, {
  formType: FormType.ROOT,
  // ...
  handleEmissionRate: NGX_SUB_FORM_HANDLE_VALUE_CHANGES_RATE_STRATEGIES.debounce(300),
});

I'm wondering: Who's using this hook with something else than a debounce?

Could we simply turn that into 

public form = createForm<ToolHolderFilter>(this, {
  formType: FormType.ROOT,
  // ...
  emissionRateDebounce: 300
});

:question:

We've never used it for anything else different than a debounce on our side but wondering if we're missing something where it could come in handy to have a different operator?

agallardol commented 3 years ago

Hello, thank you very much for the library, in some of our projects in Cotalker we implemented a lot of boilerplate similar to that of ngx-sub-forms to be able to build "subforms" in the cleanest way possible.

Some days ago we started using ngx-sub-forms and it was a good experience until we came across issue #93 , then we noticed that it had fixed it in version 5.2.0-feat-rewrite.1 and we thought OK let's install the version 5.2.0-feat-rewrite.8 which should be the most appropriate before breaking changes. We did all the necessary refactors but when trying to use 5.2.0-feat-rewrite.8 the angular builder threw us the following error:

ERROR in The target entry-point "ngx-sub-form" has missing dependencies: [ng] - ngx-observable-lifecycle [ng] - tsdef

So we thought, OK maybe it is more appropriate to use the latest version of rewrite (6.0.0-feat-rewrite.5)?

For this reason, before continuing to implement code, I would like to ask you what is your view on this. Say, from what version should we start using ngx-sub-form? We wouldn't want to have to deal with the aforementioned 5.1.2 bugs or have to refactor our code when you release 6.X.X.

The library's new approach in "feat-rewrite" branch that relies on composition rather than inheritance looks great. Thank you very much for the tremendous work you do.

maxime1992 commented 3 years ago

Hi @agallardol thanks for the kind words and happy to see you starting using ngx-sub-form!

There's a comment here: https://github.com/cloudnc/ngx-sub-form/pull/188#pullrequestreview-499443297 where indeed this was reported and we haven't fixed this yet. In the meantime you can simply add those packages directly as a quick fix with npm or yarn and you'll be fine.

About the new published versions 5.1.2-feat-rewrite.* and 6.0.0-feat-rewrite.* here's some clarification:

Now, if you feel adventurous and you're happy to help us test out the pre release I'd definitely recommend to be on latest 6.0.0-feat-rewrite.*. This is the most recent and this is what we're going to build on from now on (till of course we finally merge the rewrite!).

We've migrated one of the most important bits of our internal app and containing a lot of (ngx-sub-form) forms a month ago or so. We've discovered issues that have been fixed and we're fairly happy with it so far. But once again, this is a pre release so if you see anything going wrong please report here :).

On the documentation and migration topic, we've started to work on the documentation to help people migrate to the new version asap. The doc is not ready yet even as a draft but we're working onto it. In the meantime, your best bet is to simply look at our examples here: https://github.com/cloudnc/ngx-sub-form/tree/feat-rewrite/src/app/main-rewrite We've copied the demo app (main folder --> main-rewrite) and you'll be able to get a sense of how things should look like by looking at all the forms in that new folder. None of the existing concepts have change (root forms, sub forms, etc). It's just a matter of writting things slightly differently to get rid of inheritance but we tried to keep the same names etc. The biggest change is that to create a form we do that through a function and instead of having different methods in your class to define the form you now pass it as a param of this function.

If you're missing some info from those examples, feel free to ping here too while the documentation isn't ready and don't be afraid to share some feedback on the new API :smile:!

The library's new approach in "feat-rewrite" branch that relies on composition rather than inheritance looks great. Thank you very much for the tremendous work you do.

Much appreciated by @zakhenry and myself! Thanks :pray:

Waterstraal commented 3 years ago

Hey, I was feeling adventurous and decided to try the latest published version: 6.0.0-feat-rewrite.15. I have to say, I really like the new api; much cleaner imho!

Hower, I do have 1 question: Like @agallardol also mentioned, I was also taken by surprise that I had to install 2 additonal packages, that I did not need to install before: ngx-observable-lifecycle & tsdef. Will these 2 dependencies still be needed in the final version? I feel like this library would be much better as a stand-alone lib without external dependencies (apart from Angular of course).

Cheers!

maxime1992 commented 3 years ago

I was feeling adventurous and decided to try the latest published version: 6.0.0-feat-rewrite.15

:smile_cat: :clap:!

I have to say, I really like the new api; much cleaner imho!

Awesome, thanks for letting us know!

I had to install 2 additonal packages, that I did not need to install before: ngx-observable-lifecycle & tsdef. Will these 2 dependencies still be needed in the final version? I feel like this library would be much better as a stand-alone lib without external dependencies (apart from Angular of course).

Oh yep we've probably just forgotten to add them as peer dependencies so they auto install :zipper_mouth_face: Will do.

If you start using the new API everywhere and migrate your existing forms to it let us know more along the way if it's smooth, if you encounter any issue etc. May help us write some better migration notes :)

Waterstraal commented 2 years ago

Hi @maxime1992,

Is this project still under active development?

maxime1992 commented 2 years ago

Hello @Waterstraal. I can't say it's active I'm afraid. The refactor has been going for over a year without being merged to master.

That said. We do use ngx-sub-form regularly in our codebase at work and maintain the library. But mostly based on our own planning when we need things.

It means that the beta branch and release are still quite active, see https://github.com/cloudnc/ngx-sub-form/pull/188

What's left before we can merge the beta branch into master is:

Assuming you're already familiar with ngx-sub-form API, you probably don't need any documentation to start using the beta version (available on npm). You can just take a look here: https://github.com/cloudnc/ngx-sub-form/tree/feat-rewrite/src/app and compare the 2 folders main and main-rewrite which are the same demo app but main written with the old api and main-rewrite being written with the new api.

Hopefully at some point we find some motivation to update at least the documentation.
Just lots of things have been going on in the last year for both Zak and I and we haven't had much time to dedicate to OSS.

I think I'll have a go soon at upgrading for ng 13 though so that people using ngx-sub-form aren't stuck and can at least use the beta branch with their project. Note: Even if it's called beta it's stable now and we don't plan on introducing any further breaking change I think.

maxime1992 commented 2 years ago

@Waterstraal I've raised a MR to add support for Angular 13 and avoid the ngcc step for ngx-sub-form :)

Note: This MR targets the feat-rewrite branch (the one published on the beta channel).

maxime1992 commented 2 years ago

Alright, I've tried to find some time... and strength to rewrite the README to explain what's ngx-sub-form, how to migrate to the new api if you've been using the old one and also explain all the different principles for the lib.

Meaning, we should now be close to a state where we'll only be missing some unit tests. I think we should be good to release on master because the old API is still here and nothing will be broken by default but people should be able to start using and/or migrating to the new one.

For anyone interested, please have a look here: https://github.com/cloudnc/ngx-sub-form/pull/231

Probably not perfect but I hope it'll be at least a good start. I've been too head down into this to actually realize whether its any good or not and no one new to the project would understand what I wrote. Don't be shy and let a comment directly on the MR to let me know what you think, or even raise a MR targeting mine if you think you can improve it.

Waterstraal commented 2 years ago

👍 sounds good! I'll try to have a look next week 👏

maxime1992 commented 2 years ago

Good evening everyone :wave:

Looks like after a year and a half... We're finally merging this :tada:

Feel free to give us some feedback how the new API is working out for you. Hope you enjoy the bug fixes and the switch from inheritance to a function createForm (see updated README!) :smile_cat:.

Thanks to everyone who participated in this thread to give feedback and ideas :clap: and a big thanks for all your patience as well :pray:!

github-actions[bot] commented 2 years ago

:tada: This issue has been resolved in version 5.2.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: