-
Notifications
You must be signed in to change notification settings - Fork 33
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
feat: fix angular 14.2 support #276
feat: fix angular 14.2 support #276
Conversation
… 14.2. Calling markDirty is not needed any more to support OnPush change detection: angular/angular#10816
Hi @Waterstraal thanks for looking into this! I'll try to run that version against our internal codebase tomorrow to make sure it doesn't break any form :) |
Thanks, it's still WIP; it's not working 100% correct yet 😅. I'll give you a heads up when I've managed to fix it. Ps: on my local machine 9 unit test specs fail; even on master. Is that my local setup with windows, or do you have the same? I saw all green tests on gh actions, so not sure what's going on there. |
I've edited the PR title with a WIP prefix, please remove it from the title once it's ready :)
I do have the same indeed. I double checked the pipelines and all the tests are passing though not sure whats going on there either. As it's the case on master don't hold that into account for your branch, we'll see if CI is happy with it but this should just be fixed upstream on master not in your branch :) You can try to run the e2e tests at least, they've got a decent coverage too |
@maxime1992 can you review this PR? Thanks! |
956aae0
to
e76f270
Compare
Code LGTM. I'll go through some testing but it may take up to a week as we're very busy atm please bear with me. |
94ed973
to
d01cd69
Compare
Small update, I've published yesterday a release for that branch and got our internal CI pipeline running based on that ngx-sub-form version. Good news is, all the e2e tests are passing on all of our apps 🎉 There's just a tiny issue with 1 e2e test but I don't have the time to investigate atm but until I figure out why that unit test is now broken I'll need to put the merge of that MR on hold. I'll try to have a look next week |
@Waterstraal I've figured out why the unit I mentioned above was failing and it's all good, so I'm merging this. Thanks for taking the time to look into the upgrade! |
Fixes #272