fix(forms): include null in return types of .parent of abstract control by lazarljubenovic · Pull Request #32671 · angular/angular

lazarljubenovic · 2019-09-13T15:19:27Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

AbstractControl#parent returns a defined value.

Issue Number: #16999

What is the new behavior?

AbstractControl#parent can return undefined.

Does this PR introduce a breaking change?

Yes
No

Technically yes since we're changing the public API but then again, we're fixing a lie we told users before.

Other information

lazarljubenovic · 2019-09-14T12:41:25Z

I can't find any info on changing golden files in developer documentation. Am I supposed to do it, or someone from the team?

Splaktar · 2019-10-20T19:12:47Z

I can't find any info on changing golden files in developer documentation. Am I supposed to do it, or someone from the team?

That's a good point. The contributor docs seem to be lacking in this area. I've opened up #33278 to track that.

You need to run bazel run //tools/public_api_guard:forms_api.accept in order to update the API Golden file. Then you need to amend your commit and force push up your changes.

If you don't have Bazel installed globally in your CLI, I think that yarn bazel run //tools/public_api_guard:forms_api.accept should work.

AndrewKushnir · 2020-05-27T19:10:21Z

Hi @lazarljubenovic, thanks for creating this PR! 👍

The change looks good and as you mentioned it'd be a breaking change (so it can be included only into the major version) and I've added a label to indicate that.

One of the next steps would be to estimate the amount of effort it would take to land this inside Google's codebase. Could you please rebase this PR and resolve conflicts, so I can run internal tests to see if any cleanup is needed? Please let me know if you need help with golden files update (you should see the command as the output of one of the failing CI jobs once you rebase).

Thank you.

AndrewKushnir · 2020-06-02T00:56:28Z

Hi @lazarljubenovic, thanks for rebasing this PR and resolving conflicts!

I've started internal google checks (and more broad internal checks) and will let you know what the next steps are once we estimate the amount of effort required to land the change.

Couple comments/questions after reviewing the proposed change:

I noticed that you've used undefined as a possible type for parent property, but it looks like null might be a bit more consistent with other fields (like errors).
We should probably consider changing setParent function signature to allow for undefined value. From that perspective, setting setParent(null) would be a bit better than setParent(undefined). Note: there is also related PR fix(forms): clear parent when removing controls (#29517) #29580 that proposes adding setParent(null) calls in a couple cases. What do you think? Update: I think we should not extend setParent argument type in this PR (as I originally proposed). If we decide to do that, it should be a separate PR that should also include tests (on what setParent(null) means for different scenarios).

Thank you.

AndrewKushnir · 2020-06-02T19:35:30Z

@lazarljubenovic just a quick update: it turned out that this change is quite break-y in g3. I will try to estimate the effort on g3 side and will let you know what the next steps are. Thank you.

jelbourn

LGTM

Reviewed-for: public-api

alfaproject · 2020-09-28T23:00:27Z

  private _parentMarkedDirty(onlySelf?: boolean): boolean {
    const parentDirty = this._parent && this._parent.dirty;
-    return !onlySelf && parentDirty && !this._parent._anyControlsDirty();
+    return !onlySelf && !!parentDirty && !this._parent!._anyControlsDirty();


Now that parent can't be undefined, I think this goes more in line with recent developments:

const parentDirty = this._parent !== null && this._parent.dirty; return !onlySelf && parentDirty && !this._parent!._anyControlsDirty();

TS might even be smart enough for you to not need the ! on this._parent, but not sure

TS is not smart enough without an explicit guard function: playground.

But you could drop the const parentDirty statement since it is a bit superfluous now:

return !onlySelf && this._parent !== null && this._parent.dirty && !this._parent._anyControlsDirty();

We are very close to landing this PR (that required some cleanup in Google's codebase), so I think it'd be great to change this code in a followup refactoring PR (i.e. keep the current code as is for now so we don't need to re-run all tests again).

As of angular#32671, the type of `AbstractControl.parent` can be null which can cause compilation errors in existing apps. These changes add a migration that will append non-null assertions to existing unsafe accesses. ```` // Before console.log(control.parent.value); // After console.log(control.parent!.value); ``` The migration also tries its best to avoid cases where the non-null assertions aren't necessary (e.g. if the `parent` was null checked already).

This PR is created for testing purposes only to check angular/angular#32671 PR with listed applications.

AndrewKushnir · 2020-09-30T00:42:37Z

Quick update: I ran tests in the ngcc-validation repo and they went well. We'll need to run one last test in Google's codebase and we'll be ready to land this PR later this week (after upcoming 11.0.0-next.4 release).

AndrewKushnir · 2020-09-30T16:13:16Z

Note to Caretaker: this PR is ready to go, all tests are passing (including a global TAP) and all approvals are granted. The CI indicates 4 pending reviews, but I'm unable to remove them (looks like a glitch in GitHub UI).

As of #32671, the type of `AbstractControl.parent` can be null which can cause compilation errors in existing apps. These changes add a migration that will append non-null assertions to existing unsafe accesses. ```` // Before console.log(control.parent.value); // After console.log(control.parent!.value); ``` The migration also tries its best to avoid cases where the non-null assertions aren't necessary (e.g. if the `parent` was null checked already). PR Close #39009

angular-automatic-lock-bot · 2020-11-05T16:37:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

lazarljubenovic requested a review from a team September 13, 2019 15:19

googlebot added the cla: yes label Sep 13, 2019

lazarljubenovic force-pushed the fix-16999 branch from 2195e1d to 944c759 Compare September 13, 2019 16:05

kara added the area: forms label Sep 13, 2019

ngbot Bot added this to the needsTriage milestone Sep 13, 2019

lazarljubenovic force-pushed the fix-16999 branch from 944c759 to 27001a9 Compare September 13, 2019 20:15

pullapprove Bot requested a review from AndrewKushnir February 21, 2020 23:29

AndrewKushnir added forms: types cross-cutting: types and removed forms: types labels May 9, 2020

AndrewKushnir added breaking changes action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels May 27, 2020

Splaktar changed the title ~~fix(forms): include undefinde in .parent of abstract control~~ fix(forms): include undefined in return types of .parent of abstract control May 28, 2020

lazarljubenovic force-pushed the fix-16999 branch 2 times, most recently from e8a934b to 9602b54 Compare May 31, 2020 19:11

pullapprove Bot requested a review from kara May 31, 2020 19:11

rafaelss95 reviewed May 31, 2020

View reviewed changes

Comment thread packages/forms/src/model.ts Outdated

AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Jun 2, 2020

AndrewKushnir removed the request for review from kara June 3, 2020 23:17

pullapprove Bot requested review from IgorMinar, alxhub, jelbourn, kara and pkozlowski-opensource June 3, 2020 23:17

AndrewKushnir removed the request for review from petebacondarwin September 28, 2020 19:32

pullapprove Bot requested review from IgorMinar, petebacondarwin and pkozlowski-opensource September 28, 2020 19:32

jelbourn approved these changes Sep 28, 2020

View reviewed changes

AndrewKushnir removed the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 28, 2020

alfaproject reviewed Sep 28, 2020

View reviewed changes

petebacondarwin approved these changes Sep 29, 2020

View reviewed changes

AndrewKushnir added the risk: medium label Sep 29, 2020

AndrewKushnir added a commit to AndrewKushnir/ngcc-validation that referenced this pull request Sep 29, 2020

build: test angular/angular#32671

f241518

This PR is created for testing purposes only to check angular/angular#32671 PR with listed applications.

AndrewKushnir mentioned this pull request Sep 29, 2020

build: test angular/angular#32671 angular/ngcc-validation#1932

Closed

AndrewKushnir added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 30, 2020

josephperrott closed this in f4f1bcc Oct 5, 2020

AndrewKushnir mentioned this pull request Oct 28, 2020

fix(forms): clear parent when removing controls (#29517) #29580

Closed

14 tasks

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Nov 5, 2020

lazarljubenovic deleted the fix-16999 branch November 5, 2020 16:38

Navigation Menu

Uh oh!

fix(forms): include null in return types of .parent of abstract control - #32671

fix(forms): include null in return types of .parent of abstract control#32671
lazarljubenovic wants to merge 1 commit into
angular:masterfrom
lazarljubenovic:fix-16999

lazarljubenovic commented Sep 13, 2019

Uh oh!

lazarljubenovic commented Sep 14, 2019

Uh oh!

Splaktar commented Oct 20, 2019 •

edited

Loading

Uh oh!

AndrewKushnir commented May 27, 2020 •

edited

Loading

Uh oh!

Uh oh!

AndrewKushnir commented Jun 2, 2020 •

edited

Loading

Uh oh!

AndrewKushnir commented Jun 2, 2020 •

edited

Loading

Uh oh!

jelbourn left a comment

Uh oh!

alfaproject Sep 28, 2020

Uh oh!

petebacondarwin Sep 29, 2020

Uh oh!

AndrewKushnir Sep 29, 2020

Uh oh!

AndrewKushnir commented Sep 30, 2020

Uh oh!

AndrewKushnir commented Sep 30, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Uh oh!

Conversation

lazarljubenovic commented Sep 13, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

lazarljubenovic commented Sep 14, 2019

Uh oh!

Splaktar commented Oct 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

AndrewKushnir commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

alfaproject Sep 28, 2020

Choose a reason for hiding this comment

Uh oh!

petebacondarwin Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir Sep 29, 2020

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Sep 30, 2020

Uh oh!

AndrewKushnir commented Sep 30, 2020

Uh oh!

angular-automatic-lock-bot Bot commented Nov 5, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Splaktar commented Oct 20, 2019 •

edited

Loading

AndrewKushnir commented May 27, 2020 •

edited

Loading

AndrewKushnir commented Jun 2, 2020 •

edited

Loading

AndrewKushnir commented Jun 2, 2020 •

edited

Loading