fix(NgClass): throw a descriptive error when CSS class is not a string #12662
Conversation
pkozlowski-opensource
added
action: review
| fixture = createTestComponent(`<div [ngClass]="['foo', {}]"></div>`); | ||
| expect(() => fixture.detectChanges()) | ||
| .toThrowError( | ||
| /NgClass can only toggle CSS classes expressed as strings, got \[object Object\]/); |
There was a problem hiding this comment.
@vicb amended, although output is the same in this particular case.
There was a problem hiding this comment.
Does it really help ?
Should the check rather be in set ngClass() when the value isListLikeIterable ?
There was a problem hiding this comment.
I don't think this would be enough since someone could do [ngClass]="{foo: {}}" or sth similar. The check in this place assures that whatever comes from a collection of any type gets cough and reported.
There was a problem hiding this comment.
There should be no pb with [ngClass]="{foo: {}}", {} being turthy, right ?
There was a problem hiding this comment.
Right! I don't know what I was thinking... Amended the PR to only check array elements and do so only when those are added.
vicb
added
pr_state: LGTM
action: cleanup
pkozlowski-opensource
force-pushed
the
ng_class_not_a_string
branch
from
9e54c01
to
3ff867d
Compare
pkozlowski-opensource
removed
the
action: cleanup
pkozlowski-opensource
force-pushed
the
ng_class_not_a_string
branch
from
3ff867d
to
74f72d2
Compare
| } else { | ||
| throw new Error( | ||
| `NgClass can only toggle CSS classes expressed as strings, got ${stringify(record.item)}`); | ||
| } |
There was a problem hiding this comment.
Wondering if
if (record.item) {
this._toggleClass(`${record.item}`, true);
}would be enough here ?
That's not ideal but throwing at runtime isn't either ?
There was a problem hiding this comment.
Yeh, this is an option I was considering as well... Should be enough (and certainly less expensive). Let's do this?
There was a problem hiding this comment.
On the other hand it will just throw from the DOM with sth like:
VM834:1 Uncaught DOMException: Failed to execute 'add' on 'DOMTokenList': The token provided ('[object Object]') contains HTML space characters, which are not valid in tokens.(…)
which is not great either...
There was a problem hiding this comment.
Then may be only omit the else branch ?
There was a problem hiding this comment.
IMO this would be super-confusing as you wouldn't know what is going on. I would rather prefer to do nothing that swallow non-string items silently.
There was a problem hiding this comment.
Maybe we could use the console service to log a warning ?
There was a problem hiding this comment.
This would differ from how we handle invalid input for other cases. IMO the viable options are:
- throw as this PR
- do nothing (leave as is)
- stringify
Throwing as in this PR is nicest to users. Do you have any particular / precise concerns with this approach? My second option would be to stringify if so.
pkozlowski-opensource
added
the
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #12586