fix(common): allow null/undefined to be passed to ngClass input (#39280)

[imirkin]

With strict template type checking, a null/undefined value will raise an
error. However the implementation is completely fine with it, and it
would be pointless to "fix" it at the callsite and convert to e.g. an
empty string. Allow all of the "supported types" to be passed in
directly to ngClass.

Fixes #39280

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/angular/angular/blob/main/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?

Issue Number: #39280

Currently bindings to [ngClass] that may return null or undefined are rejected by typescript as a result of enabling strictTemplates (e.g. map.get('foo') or anything else whose type includes undefined)

What is the new behavior?

undefined/null are allowed by the @Input type and thus strictTemplate-enabled builds

Does this PR introduce a breaking change?

• Yes
• No

Wasn't sure how to add a test for this specifically, since one has to enable strictTemplates for the source issue to occur. Suggestions welcome.

[imirkin]

Looks like the google3 presubmit check is failing, but I don't have access to the details (having left G ~14y ago...). Happy to fix whatever's needed if I could get some details on what's failing.

Also, not 100% sure how this will look in the public docs (i.e. https://angular.io/api/common/NgClass), so let me know if I should flip it back to the explicit types and just add |null|undefined to the end.

[pkozlowski-opensource]

The proposed change would expose NgClassSupportedTypes to the public API surface. NgClassSupportedTypes is not part of the public API currently and we really don't want to expose it since it would enlarge the public API.

Could you add null / undefined to the ngClass setter instead?

[imirkin]

The proposed change would expose NgClassSupportedTypes to the public API surface. NgClassSupportedTypes is not part of the public API currently and we really don't want to expose it since it would enlarge the public API.

Could you add null / undefined to the ngClass setter instead?

Sure thing. Done.

[AndrewKushnir]

Just a quick comment: I think it'd be also great to add some tests into the packages/common/test/directives/ng_class_spec.ts file to check for undefined case (there are some existing tests there for null already).

[imirkin]

Just a quick comment: I think it'd be also great to add some tests into the packages/common/test/directives/ng_class_spec.ts file to check for undefined case (there are some existing tests there for null already).

Just like copy the null ones, and s/null/undefined, or something more creative?

[Edit: unless I hear otherwise, I'll do that tomorrow (US/Eastern) morning.]

[AndrewKushnir]

@imirkin it should be sufficient to copy the null-based test cases and use undefined instead.

[imirkin]

@imirkin it should be sufficient to copy the null-based test cases and use undefined instead.

Done. [Note, there are a bunch of tests that use null to "clear" out the value, but I didn't think it was important to make a copy of those tests to check for undefined clearing out the old value. There were also tests for string -> null, which I could add for string -> undefined, but again, didn't seem relevant. Let me know if you feel otherwise and I can adjust easily enough.]

[AndrewKushnir]

@imirkin thanks for adding a test. I think it'd be great to have a similar set of tests for null and undefined (i.e. cover the cases that you've mentioned), just to make sure all code paths are covered (and there are no places like === null that would work for null only). As an option, you can just extend existing tests to add one more check there (so both null and undefined cases are co-located).

[imirkin]

@AndrewKushnir how's this update?

[AndrewKushnir]

@imirkin the change looks good, thanks for adding extra tests 👍

I've been looking at the NgStyle as well and found out that there is only null there in the input type, but there is no undefined:
https://github.com/angular/angular/blob/main/packages/common/src/directives/ng_style.ts#L59

I'm wondering if we should just add null as a type to the NgClass for now (in this PR) to achieve consistency with NgStyle and investigate the undefined case a bit more to see if this is used in other FW APIs in the same way.

@pkozlowski-opensource WDYT?

[imirkin]

@AndrewKushnir Well, what I really need is "undefined". I don't need null. I was just adding it in because it was (a) already being used and (b) seemed like the right thing.

I can add another commit here adding undefined to NgStyle if you like.

[pkozlowski-opensource]

@AndrewKushnir @imirkin IMO we can handle NgStyle in a separate PR, no need to delay / making this one bigger.

[imirkin]

@AndrewKushnir @pkozlowski-opensource so this is all good? or more work to be done? Let me know either way, happy to make additional changes, but I really do want the "undefined" bit of it...

[pkozlowski-opensource]

LGTM

Reviewed-for: public-api
Reviewed-for: fw-core

[pkozlowski-opensource]

LGTM

Reviewed-for: public-api
Reviewed-for: fw-common

[atscott]

reviewed-for: public-api

[jessicajaniuk]

reviewed-for: fw-common, public-api

[AndrewKushnir]

This PR was merged into the repository by commit e2ab99b.

[AndrewKushnir]

@imirkin thanks for improving typings 👍 If you get a chance to add undefined to the ngStyle (in a separate PR) for symmetry - that'd be great.

[angular-automatic-lock-bot]

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