Skip to content

feat(forms): add migration for AbstractControl.parent accesses - #39009

Closed
crisbeto wants to merge 2 commits into
angular:masterfrom
crisbeto:abstract-control-parent-migration
Closed

feat(forms): add migration for AbstractControl.parent accesses#39009
crisbeto wants to merge 2 commits into
angular:masterfrom
crisbeto:abstract-control-parent-migration

Conversation

@crisbeto

@crisbeto crisbeto commented Sep 26, 2020

Copy link
Copy Markdown
Member

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).

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms area: migrations Issues related to `ng update`/`ng generate` migrations target: major This PR is targeted for the next major release labels Sep 27, 2020
@ngbot ngbot Bot modified the milestone: needsTriage Sep 27, 2020
@crisbeto
crisbeto marked this pull request as ready for review September 27, 2020 05:29
@pullapprove
pullapprove Bot requested a review from gkalpak September 27, 2020 05:29

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks @crisbeto 👍

Comment thread packages/core/schematics/migrations/abstract-control-parent/README.md Outdated
Comment thread packages/core/schematics/migrations/abstract-control-parent/util.ts Outdated
Comment thread packages/core/schematics/migrations/abstract-control-parent/util.ts Outdated
@AndrewKushnir AndrewKushnir added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 28, 2020
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).
@crisbeto
crisbeto force-pushed the abstract-control-parent-migration branch from 0f86d99 to ba330c6 Compare September 29, 2020 06:14
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: review The PR is still awaiting reviews from at least one requested reviewer labels Sep 29, 2020

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for addressing the feedback @crisbeto 👍

@AndrewKushnir AndrewKushnir added the action: presubmit The PR is in need of a google3 presubmit label Sep 29, 2020
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Presubmit.

@AndrewKushnir AndrewKushnir added state: blocked and removed action: presubmit The PR is in need of a google3 presubmit labels Sep 30, 2020
@AndrewKushnir

Copy link
Copy Markdown
Contributor

FYI, adding the "blocked" label for now since the type change should be merged first.

@josephperrott josephperrott removed the action: merge The PR is ready for merge by the caretaker label Oct 2, 2020
@crisbeto

crisbeto commented Oct 6, 2020

Copy link
Copy Markdown
Member Author

The related forms PR has been merged in. I'm unblocking this one.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed state: blocked labels Oct 6, 2020

@gkalpak gkalpak left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Reviewed-for: dev-infra

@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker area: forms area: migrations Issues related to `ng update`/`ng generate` migrations cla: yes target: major This PR is targeted for the next major release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants