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(common): remove non-null assertions from directives #48476

Closed
wants to merge 4 commits into from
Closed

refactor(common): remove non-null assertions from directives #48476

wants to merge 4 commits into from

Conversation

JeanMeche
Copy link
Member

PR Checklist

Per request from @pkozlowski-opensource, I'm splitting PR #48241 into 2.

See individual commits.

These are fixes targeted to resolve #24571

PR Type

What kind of change does this PR introduce?

  • Refactoring (no functional changes, no api changes)

Does this PR introduce a breaking change?

  • No

@pullapprove pullapprove bot requested a review from atscott December 13, 2022 17:22
atscott
atscott requested changes Dec 13, 2022
View reviewed changes
packages/common/src/directives/ng_plural.ts Outdated
@@ -71,8 +68,10 @@ export class NgPlural {
this._clearViews();

const cases = Object.keys(this._caseViews);
const key = getPluralCategory(this._switchValue, cases, this._localization);
this._activateView(this._caseViews[key]);
if (this._switchValue !== undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would _switchValue be better with a default value? It's actually kind of correct with the ! right now. The directive won't match unless this is defined. It's a little unfortunate that we have to add code to account for the possibility of undefined when it really can't ever be in production.

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, we can keep the type safety by passing the value to _updateView

packages/common/src/directives/ng_switch.ts Outdated
@@ -106,8 +106,7 @@ export class SwitchView {
standalone: true,
})
export class NgSwitch {
// TODO(issue/24571): remove '!'.
private _defaultViews!: SwitchView[];
private _defaultViews?: SwitchView[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this have a default of empty array instead of undefined?

Copy link
Member Author

@JeanMeche JeanMeche Dec 21, 2022 •
edited

Choose a reason for hiding this comment

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

I'd rather see undefined or null as a default value. It's created when needed by _addDefault and checked by _updateDefaultCases.

I'm always to other point of views though !

Copy link
Contributor

Choose a reason for hiding this comment

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

Having it be nullable rather than empty list provides no additional meaning. I feel quite strongly that this should never be null or undefined. You can also update the rest of the code to remove the truthiness checks of this value.

Copy link
Member Author

Choose a reason for hiding this comment

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

I used empty array as default value. It seems presubmit is failing now with the changes. Any heads-up ?

@jessicajaniuk jessicajaniuk added the area: common Issues related to APIs in the @angular/common package label Dec 13, 2022
@ngbot ngbot bot added this to the Backlog milestone Dec 13, 2022
@JeanMeche JeanMeche force-pushed the feature/fix-24571-directives branch 3 times, most recently from a6014c4 to 2478c77 Compare December 21, 2022 22:39
atscott
atscott approved these changes Dec 27, 2022
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@JeanMeche
refactor(common): remove non-null assertions
9355cd3 
As part of angular#24571, removing all non-null assertions from common/directives
@JeanMeche JeanMeche force-pushed the feature/fix-24571-directives branch 5 times, most recently from aa2afb1 to b883f2a Compare December 27, 2022 19:18
atscott
atscott approved these changes Dec 28, 2022
View reviewed changes
Copy link
Contributor

@atscott atscott left a comment

Choose a reason for hiding this comment

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

@JeanMeche looks like the lint test is failing. The code changes otherwise look good and the internal test failures are unrelated

@angular-robot angular-robot bot requested a review from atscott December 29, 2022 16:38
@JeanMeche
refactor(common): remove non-null assertions
9173755 
As part of angular#24571, removing all non-null assertions from common/directives
@JeanMeche
refactor(common): code cleaning
f1a0a9d 
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)
@JeanMeche
refactor(common): remove non-null assertions
51bd989 
As part of angular#24571, removing all non-null assertions from common/test/directives
@JeanMeche
Copy link
Member Author

@JeanMeche looks like the lint test is failing. The code changes otherwise look good and the internal test failures are unrelated

Done.

@atscott atscott added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jan 5, 2023
@alxhub
Copy link
Member

alxhub commented Jan 5, 2023

This PR was merged into the repository by commit d887d69.

alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
23b465e 
As part of #24571, removing all non-null assertions from common/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
d832cb2 
As part of #24571, removing all non-null assertions from common/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): code cleaning (#48476)
a8ba265 
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
f672d8f 
As part of #24571, removing all non-null assertions from common/test/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
7d30528 
As part of #24571, removing all non-null assertions from common/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
be321f0 
As part of #24571, removing all non-null assertions from common/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): code cleaning (#48476)
406ae85 
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
f3babe0 
As part of #24571, removing all non-null assertions from common/test/directives

PR Close #48476
@alxhub alxhub closed this in 559f518 Jan 5, 2023
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
c29e462 
As part of #24571, removing all non-null assertions from common/directives

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): code cleaning (#48476)
ef57874 
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close #48476
alxhub pushed a commit that referenced this pull request Jan 5, 2023
@JeanMeche @alxhub
refactor(common): remove non-null assertions (#48476)
d887d69 
As part of #24571, removing all non-null assertions from common/test/directives

PR Close #48476
@JeanMeche JeanMeche deleted the feature/fix-24571-directives branch January 7, 2023 01:33
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@JeanMeche @trekladyone
refactor(common): remove non-null assertions (angular#48476)
19e485f 
As part of angular#24571, removing all non-null assertions from common/directives

PR Close angular#48476
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@JeanMeche @trekladyone
refactor(common): remove non-null assertions (angular#48476)
8d57958 
As part of angular#24571, removing all non-null assertions from common/directives

PR Close angular#48476
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@JeanMeche @trekladyone
refactor(common): code cleaning (angular#48476)
c135e3a 
* removing guard as console.warn is now widely supported
* Couldn't remove non-null assertion, waiting for TS support of getters with different types (microsoft/TypeScript#43662)

PR Close angular#48476
trekladyone pushed a commit to trekladyone/angular that referenced this pull request Feb 1, 2023
@JeanMeche @trekladyone
refactor(common): remove non-null assertions (angular#48476)
c523063 
As part of angular#24571, removing all non-null assertions from common/test/directives

PR Close angular#48476
@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 Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@atscott atscott atscott approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: common Issues related to APIs in the @angular/common package 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.

Post strictPropertyInitialization flag flip cleanup
4 participants
@JeanMeche @alxhub @atscott @jessicajaniuk