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

Upgrade to Angular 15 #282

Closed ophirKatz closed 1 year ago

188599 commented 1 year ago

Support for Angular 15 would be great.

I added this lib to my Angular 15 project but I had to manually change several typings for it to compile. Weird, considering that typed forms were already part of Angular 14, had to add extends { [key: string]: any; } constraint to all generics, otherwise it would break the build.

maxime1992 commented 1 year ago

Hello. I've cut a first release (7.0.0) which removes the deprecated API.

I then tried to give a quick go at the upgrade but it's a bit more painful than I thought and gives me errors around our wrapper types (that were written before Angular forms were typed). I don't think it'll be a quick fix and it needs a bit of a dive to understand whether we still need our wrapper types, if we should only use the ones from Angular, etc etc.

I've raised a draft PR that anyone can take a look at. If you've got ideas/opinions on the most appropriate way to deal with the new typed forms please let me know.

I'm afraid I had to stop here for potentially a while. I wanted to give it a quick go but it needs more work. If anyone is willing to take a look at my PR and eventually give it a go to continue it, please checkout from my branch and raise a PR on your side either targetting my branch or master directly. Thanks!

188599 commented 1 year ago

From what I have seem, most of the things this lib has already covers the new things the official typed forms Angular offers, so I don't really see a need to rush for a change into the new typings.

I'd suggest to keep it simple at first and just change the constraints necessary for the current types from the lib to work well with the new constraints in the new Angular types, and gradually move towards changing things in the backend to the new APIs.

Edit: Looking a bit further into it, the only thing that's broken in the current type seems to be that the type of value in the FormGroup.setValue needs to be of type object.

Changing the two interfaces in ngx-sub-form-utils.ts to following seems to fix the build.

...
export interface TypedFormGroup<TValue> extends UntypedFormGroup {
    value: TValue;
    valueChanges: Observable<TValue>;
    controls: ControlsType<TValue>;
    setValue(value: TValue & {}, options?: Parameters<UntypedFormGroup['setValue']>[1]): void;
    patchValue(value: Partial<TValue>, options?: Parameters<UntypedFormGroup['patchValue']>[1]): void;
    getRawValue(): TValue;
}
...
export interface TypedFormControl<TValue> extends UntypedFormGroup {
    value: TValue;
    valueChanges: Observable<TValue>;
    setValue(value: TValue & {}, options?: Parameters<UntypedFormControl['setValue']>[1]): void;
    patchValue(value: Partial<TValue>, options?: Parameters<UntypedFormControl['patchValue']>[1]): void;
}
...
github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 8.0.0-feat-upgrade-angular-15.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

maxime1992 commented 1 year ago

Hey @188599 thanks for the snippets. I had a quick look, it wasn't enough to get it to compile fully but I pushed a little bit more and managed to get to a good state I believe. If you could make a review to my PR that'd be much appreciated.

Also, with the new changes on angular material the view got badly broken and the e2e tests needed a bit of a refresh :nail_care:.

As for Cypress, I've faced some difficulties to make it run on CI and had to change how the tests are run a little bit but it's all done, we should be good to merge soon :crossed_fingers:

For anyone willing to give a go to the beta version with support for angular 15: Use the npm version ngx-sub-form@8.0.0-feat-upgrade-angular-15.1 for now and you should be good to go. Please give some feedback if you want to help get this merged asap :)

(as you can see, the bot above is half correct. It's "resolved" BUT not in the main branch).

188599 commented 1 year ago

Hey @188599 thanks for the snippets. I had a quick look, it wasn't enough to get it to compile fully but I pushed a little bit more and managed to get to a good state I believe. If you could make a review to my PR that'd be much appreciated. ...

I'd be glad to take a look at. And thanks for the quick update!

188599 commented 1 year ago

Perfect! Only had to update rxjs to the latest minor version. 👏👏

maxime1992 commented 1 year ago

Checked on our private monorepo at work and all the unit/e2e tests are passing, no blocker on my side. Unless someone give a shout not to merge this I'll release soon :)

ophirKatz commented 1 year ago

Thank you for the effort. I'll test it today for our repo (holding off still on migrating to angular 15 until more dependencies release upgrades, but I'll still check all things related to sub_form). I'll let you know if all is good or not

ophirKatz commented 1 year ago

I tested it on our code base. Seems like it works well. So just to be sure, there are no changes to the API, right?

maxime1992 commented 1 year ago

There shouldn't be any :+1: I just wanted to double check that the changes on the types wouldn't break anything but they shouldn't really. The breaking change is just for major bump of angular version

github-actions[bot] commented 1 year ago

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

The release is available on:

Your semantic-release bot :package::rocket: