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.

Sign up for GitHub

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

Jump to bottom

refactor(forms): eagerly initialize data members #44292

Closed
wants to merge 1 commit into from
Closed

refactor(forms): eagerly initialize data members #44292

wants to merge 1 commit into from

Conversation

theruslanusmanov
Copy link
Contributor

@theruslanusmanov theruslanusmanov commented Nov 27, 2021 •
edited by dylhunn

Data members in AbstractControl should be eagerly
initialized to address #24571. This eliminates the need to
constantly check for truthiness and makes code much more readable.

More PRs to follow to address #24571.

PR Checklist

Please check if your PR fulfills the following requirements:

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?

https://angular.io/api/forms/AbstractControl

Issue Number: 24571

What is the new behavior?

Removes '!' from AbstractControl data members

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@google-cla google-cla bot added the cla: yes label Nov 27, 2021
@pullapprove pullapprove bot requested a review from dylhunn November 27, 2021 17:24
AndrewKushnir
AndrewKushnir reviewed Nov 29, 2021
View reviewed changes
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: review The PR is still awaiting reviews from at least one requested reviewer area: forms labels Nov 29, 2021
@ngbot ngbot bot added this to the Backlog milestone Nov 29, 2021
@AndrewKushnir AndrewKushnir added the refactoring Issue that involves refactoring or code-cleanup label Nov 29, 2021
dylhunn
dylhunn reviewed Nov 29, 2021
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

Andrew left a couple comments that I agree with -- it's better to leave the ! assertions for members we know will be initialized. We can't add undefined to the types, since that's breaking.

AndrewKushnir
AndrewKushnir reviewed Nov 29, 2021
View reviewed changes
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
packages/forms/src/model.ts Outdated Show resolved Hide resolved
AndrewKushnir
AndrewKushnir approved these changes Nov 29, 2021
View reviewed changes
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

LGTM, just one comment on no longer needed type annotation.

packages/forms/src/model.ts Outdated Show resolved Hide resolved
@AndrewKushnir AndrewKushnir added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews target: patch This PR is targeted for the next patch release and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 29, 2021
@AndrewKushnir
Copy link
Contributor

@theruslanusmanov thanks for updating the PR! Could you please also merge all commits into one (into the first one)?

@theruslanusmanov
Copy link
Contributor Author

@AndrewKushnir sure, thank you!

@theruslanusmanov @AndrewKushnir
refactor(forms): eagerly initialize data members
2414563 
Data members in AbstractControl should be eagerly
initialized to address issue/24571. This eliminates the need to
constantly check for truthiness and makes code much more readable.

More PRs to follow to address issue/24571.
@AndrewKushnir
Copy link
Contributor

@theruslanusmanov thanks! I've also pushed a rebase, since the CI didn't start for some reasons previously. Once CI run is completed, we'll run tests in Google's codebase and let you know if there are any additional updates required. Thank you.

@AndrewKushnir AndrewKushnir added action: presubmit The PR is in need of a google3 presubmit and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 30, 2021
@AndrewKushnir
Copy link
Contributor

FYI, I've started tests in Google's codebase (internal-only link).

@AndrewKushnir AndrewKushnir added action: merge The PR is ready for merge by the caretaker and removed action: presubmit The PR is in need of a google3 presubmit labels Nov 30, 2021
@AndrewKushnir
Copy link
Contributor

@theruslanusmanov tests in Google's codebase were successful and I'm adding this PR to the merge queue.

Thanks for contributing to Angular 👍

dylhunn
dylhunn approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@dylhunn dylhunn left a comment

Choose a reason for hiding this comment

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

reviewed-for: fw-forms

@dylhunn
Copy link
Contributor

dylhunn commented Nov 30, 2021

This PR was merged into the repository by commit ca5b9b5.

@dylhunn dylhunn closed this in ca5b9b5 Nov 30, 2021
dylhunn pushed a commit that referenced this pull request Nov 30, 2021
@theruslanusmanov @dylhunn
refactor(forms): eagerly initialize data members (#44292)
66b73d4 
Data members in AbstractControl should be eagerly
initialized to address issue/24571. This eliminates the need to
constantly check for truthiness and makes code much more readable.

More PRs to follow to address issue/24571.

PR Close #44292
@dylhunn
Copy link
Contributor

dylhunn commented Nov 30, 2021

Thank you for your contribution @theruslanusmanov!

@theruslanusmanov
Copy link
Contributor Author

@dylhunn Thank you!

dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
@theruslanusmanov @dimakuba
refactor(forms): eagerly initialize data members (angular#44292)
1807a91 
Data members in AbstractControl should be eagerly
initialized to address issue/24571. This eliminates the need to
constantly check for truthiness and makes code much more readable.

More PRs to follow to address issue/24571.

PR Close angular#44292
@angular-automatic-lock-bot
Copy link

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 Dec 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@dylhunn dylhunn dylhunn approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: forms cla: yes refactoring Issue that involves refactoring or code-cleanup target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@theruslanusmanov @AndrewKushnir @dylhunn