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

ngx-sub-form doesn't build with Angular's Ivy compiler #134

Closed
CyberBotX opened this issue Feb 7, 2020 · 23 comments · Fixed by #145
Closed

ngx-sub-form doesn't build with Angular's Ivy compiler #134

CyberBotX opened this issue Feb 7, 2020 · 23 comments · Fixed by #145
Assignees
Labels
released scope: lib Anything related to the library itself state: has PR A PR is available for that issue state: needs repro This issue needs a repro type: bug/fix This is a bug or at least needs a fix

Comments

@CyberBotX
Copy link

So, Angular 9 just dropped today, and the Ivy compiler is now default. But if I try to build one of my Angular pages that uses ngx-sub-form while using Ivy, I get the following error:

ERROR in CommissionedArt/src/app/Components/input-form.html:1:26 - error TS2740: Type 'TypedFormGroup<InputForm>' is missing the following properties from type 'FormGroup': _parent, _asyncValidationSubscription, _updateAncestors, _setInitialStatus, and 4 more.

I don't get this if I disable Ivy and use the old View Engine compiler instead.

@maxime1992
Copy link
Contributor

Just to have everything linked together: #101, #102

I'll try to look into that asap, thanks @CyberBotX :)

@maxime1992 maxime1992 added the type: bug/fix This is a bug or at least needs a fix label Feb 7, 2020
@CyberBotX
Copy link
Author

Oh, I probably should also mention that I got this error alongside that one:

CommissionedArt/src/app/app.html:1:13 - error TS2322: Type 'InputForm | null' is not assignable to type 'InputForm'.
  Type 'null' is not assignable to type 'InputForm'.

That one I could solve by just making my dataInput type have | null on it, but it would be nice to not require that too.

maxime1992 added a commit that referenced this issue Feb 10, 2020
BREAKING CHANGES: requires angular 9

This fixes #101, fixes #102, fixes #134
@maxime1992
Copy link
Contributor

@CyberBotX can you create a minimal repro and post it on Github or as a zip?

That'd really help me working on the upgrade 🙏

@maxime1992 maxime1992 self-assigned this Feb 10, 2020
@maxime1992 maxime1992 added scope: lib Anything related to the library itself state: needs repro This issue needs a repro labels Feb 10, 2020
@CyberBotX
Copy link
Author

I can, but it might be over the weekend depending on my time. I'll try to get something up sooner if I can.

@maxime1992
Copy link
Contributor

Brilliant, thanks for your help :)

@CyberBotX
Copy link
Author

So, I tried to put together an example on StackBlitz here:

https://stackblitz.com/edit/angular-ngx-sub-form-with-ivy

But... it builds and runs just fine over there, despite that I'm using the same versions of the packages and similiar configuration as I have locally. I don't know if it is because StackBlitz isn't building with Ivy (even if "enableIvy": true is included in the configuation) or because of something else in my local setup.

maxime1992 added a commit that referenced this issue Feb 19, 2020
BREAKING CHANGES: requires angular 9

This fixes #101, fixes #102, fixes #134
maxime1992 added a commit that referenced this issue Feb 19, 2020
BREAKING CHANGES: requires angular 9

This fixes #101, fixes #102, fixes #134
@maxime1992
Copy link
Contributor

We've been working on an upgrade to ng 9 and it should be released pretty soon (v5 I think?).

Please give it a go and reopen if needed :)

I've tried the ngx-sub-form update build building it locally and use the local build within our own app (that we also just upgraded to angular 9) and it appeared to be fine 👍

@maxime1992
Copy link
Contributor

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@CyberBotX
Copy link
Author

I just updated ngx-sub-form to 5.0.0 and attempted to build one of my projects with Ivy enabled. It still gives me the same errors I mentioned in my initial 2 comments. This is even after removing my node_modules and package-lock.json, reinstalling my packages, and running ngcc as recommended by Angular's docs.

(I don't seem to be able to reopen the issue.)

@maxime1992 maxime1992 reopened this Feb 21, 2020
@maxime1992
Copy link
Contributor

@CyberBotX I downloaded your stackblitz repro and tried to run it locally, it's all fine (build time and run time)

Can you please:

  • create a new repo using ng new
  • make a minimal repro of your issue
  • create a new public repo on Github and push it
  • share the URL
    ?

That'd be really helpful to debug because I don't have that error 👍 Thanks!

@CyberBotX
Copy link
Author

Here is the repo: https://github.com/CyberBotX/ngx-sub-form-test

It's weird, the StackBlitz version of this works flawlessly. The version I put up on GitHub, though, it gives the error immediately after setting it up for me. I made the new repo as suggested, then immediately removed node_modules and package-lock.json, put in my sample code, updated package.json with the latest versions of the packages (as well as including ngx-sub-form) and making sure the ngcc postinstall script was there, reinstalled the packages, then attempted to build.

One thing I found, though, that might help with the debugging process, the errors do not appear if I remove the "strictTemplates": true option from the angularCompilerOptions of tsconfig.json. As soon as I add that back in, the error comes back.

@maxime1992
Copy link
Contributor

Thanks for the repro @CyberBotX 🙏

I dug a little into it and without even touching Angular and the AoT, here's a minimal repro:

import { FormGroup } from "@angular/forms";
import { TypedFormGroup } from "ngx-sub-form";

const a: FormGroup = (null as any) as TypedFormGroup<any>;

https://stackblitz.com/edit/typescript-hjcu3y

I do not understand why we'd be missing those properties as the type of TypedFormGroup is:

export declare type TypedFormGroup<FormInterface> = Omit<FormGroup, 'controls' | 'value'> & {
    controls: Controls<FormInterface>;
    value: FormInterface;
};

So we just start off FormGroup and make 2 properties stricter 🤷‍♂️

One thing I noticed though: All the missing properties seem to be prefixed by an underscore and that's maybe what's stripping it away?

Type 'TypedFormGroup<any>' is missing the following properties from type 'FormGroup': _parent, _asyncValidationSubscription, _updateAncestors, _setInitialStatus, and 4 more.

@maxime1992
Copy link
Contributor

@zakhenry any idea on the above? I'm quite suprised by this behavior to be honnest 🤔

Here AoT is correct, that's not a regression or anything wrong. But why are those properties not available, I have no clue.

@zakhenry
Copy link
Contributor

@maxime1992 yep it appears that Omit erases all private members types on the first arg which makes them somehow assignment-incompatible.

here's a tighter repro:

class Foo {
  public a: string;
  public b: string;
  private c: string;
}

type NarrowedFoo = Omit<Foo, 'a'> & { a: 'bar' };

const test: Foo = null as NarrowedFoo;
// Property 'c' is missing in type 'NarrowedFoo' but required in type 'Foo'.

@zakhenry
Copy link
Contributor

I suspect the reason this is now breaking is due to strict template type checking. I'm not sure if there is an easy workaround, and it looks like #139 is further using this pattern

@zakhenry
Copy link
Contributor

zakhenry commented Feb 23, 2020

Ok yep here's a way to work around it - rather than using Omit to erase the other type, we can use an interface instead to extend the base type:

class Foo {
  public a: string;
  public b: string;
  private c: string;
}

interface NarrowedFoo extends Foo {
  a: 'bar';
}

const test = null as NarrowedFoo;

console.log(test.a); // intellisense on test.a correctly gives `(property) NarrowedFoo.a: "bar"` 

const assignment: Foo = test;

@zakhenry
Copy link
Contributor

zakhenry commented Feb 23, 2020

this is obviously far from ideal, but if this is a genuine blocker, you can work around this by setting fullTemplateTypeCheck to false in

https://github.com/CyberBotX/ngx-sub-form-test/blob/99b3f4acf6b8e06376b5488fa94e956c93421645/tsconfig.json#L32

we will obviously continue to work to get ngx-sub-form working with ivy in the meantime,

@maxime1992
Copy link
Contributor

it appears that Omit erases all private members types on the first arg which makes them somehow assignment-incompatible

#134 (comment)

Wow I tried to repro that on TS playground and couldn't repro 🤔 ...

this is obviously far from ideal, but if this is a genuine blocker, you can work around this by setting fullTemplateTypeCheck to false

Loosing all the type safety offered by Angular is not really an option, we should find a way around this. If there's any workaround to be done I'd say wrap the [formGroup]="formGroup" as [formGroup]="$any(formGroup)". At least it'll keep everything else type safe.

See https://angular.io/guide/template-syntax#any-type-cast-function for more info

@zakhenry
Copy link
Contributor

@maxime1992 I've already fixed this issue - #145

@maxime1992 maxime1992 added the state: has PR A PR is available for that issue label Feb 24, 2020
@maxime1992
Copy link
Contributor

Thanks for all your help digging into that issue @CyberBotX 👍

Zak made a fix and it should be released in a few mn

@maxime1992
Copy link
Contributor

🎉 This issue has been resolved in version 5.0.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@CyberBotX
Copy link
Author

I can confirm that the update does fix the issue from my first comment. I still have the issue from my second comment but it isn't as big a deal. Thanks for getting this fixed so quickly. :D

@zakhenry
Copy link
Contributor

@CyberBotX good to hear that worked out for you. Your second issue has now become #147 if you want to follow that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released scope: lib Anything related to the library itself state: has PR A PR is available for that issue state: needs repro This issue needs a repro type: bug/fix This is a bug or at least needs a fix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants