fix(common): DatePipe doesn't throw for NaN #14117
Conversation
| @@ -103,7 +103,7 @@ export class DatePipe implements PipeTransform { | |||
| transform(value: any, pattern: string = 'mediumDate'): string { | |||
| let date: Date; | |||
|
|
|||
| if (isBlank(value)) return null; | |||
| if (isBlank(value) || (<any>Number).isNaN(value)) return null; | |||
There was a problem hiding this comment.
Why do we need cast Number to <any> here? How about using value != value (https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/isNaN)
There was a problem hiding this comment.
why not simply use isNaN(value)?
There was a problem hiding this comment.
isNaN has false-positives - check the article I've linked.
There was a problem hiding this comment.
why not simply use isNaN(value)?
Because isNaN is broken and to fix it they introduced Number.isNaN in es2015
Why do we need cast Number to here?
because angular doesn't allow to use es2015 types (at least for Object)
How about using value != value
yeah, it's a good option, just not obvious why it's required vs Number.isNaN
pkozlowski-opensource
added
the
area: common
DzmitryShylovich
force-pushed
the
gh/14103
branch
from
266a2a3
to
9a23b40
Compare
DzmitryShylovich
force-pushed
the
gh/14103
branch
from
cb3fbe4
to
8e17b7e
Compare
|
unrelated to this PR, I'm just trying to get hold of Dzmitry: hi @DzmitryShylovich, can you please email me at iminar@google ? thanks |
|
Related to this PR: LGTM |
pkozlowski-opensource
added
pr_state: LGTM
action: merge
|
@IgorMinar I sent you an email |
|
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 #14103