Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix (date filter): added !isNaN check before creating new Date object #15010

Closed
wants to merge 1 commit into from

Conversation

plong0
Copy link

@plong0 plong0 commented Aug 9, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
Please see: #13415 (comment)

not solved by PR #14221 in release 1.4.12 or 1.5.8

What is the new behavior (if this is a feature change)?
Perform !isNaN(date) check in date filter before attempting to create new Date(date) object.

Does this PR introduce a breaking change?
Possibly. If a user of the date filter expects it to throw the DateRange error if a NaN value is passed.

Please check if the PR fulfills these requirements

Other information:

Workaround is simple enough to pass values through a normalization filter before passing to date filter.

@gkalpak
Copy link
Member

gkalpak commented Aug 10, 2016

Does this PR introduce a breaking change?
Possibly. If a user of the date filter expects it to throw the DateRange error if a NaN value is passed.

LOL - That is hardly a problem. The real problem is for people not using Safari and expecting a Date object (with the appropriate methods) to be return for NaN, because now they will get a TypeError:

var value = NaN;   // some numeric value that can also be NaN

var date = dateFilter(value);
if (date.getTime() > someThreshold) {
  // Do this and that...
}

Before this change, the above snippet will work (date.getTime() will return NaN).
After this change, the above snppet will throw a TypeError (because date will be NaN and doesn't have a getTime() method).

@gkalpak
Copy link
Member

gkalpak commented Aug 10, 2016

Tbh, I don't think the benefits of this justify the cost.

The benefits are that we work-around one specific case (among several) of a bug in one specific browser.
The cost is that we introduce a small breaking change.

I don't think many apps will be affected by this, but people upgrading will nevertheless have to go through the whole codebase checking for situations where a value passed to dateFilter could be NaN.

I don't feel too strongly though. Let's see what others think.

@Narretz
Copy link
Contributor

Narretz commented Sep 8, 2016

A agree with @gkalpak

@Narretz Narretz closed this Sep 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants