Skip to content

feat(observable): coercion, typed observable #623

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

Closed
bigopon wants to merge 5 commits into from
Closed

feat(observable): coercion, typed observable #623

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d06e24a
feat(observable): coercion, typed observable
bigopon Sep 26, 2017
3b2eb53
handle coerce invalid date
Oct 16, 2017
d35935f
add test for inheritance
Oct 19, 2017
ad49a94
use isNaN to check invalid date
Oct 19, 2017
5724307
usePropType -> usePropertyType
Oct 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/coerce-functions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,14 @@ export const coerceFunctions = {
return !!a;
},
date(a) {
return new Date(a);
// Invalid date instances are quite problematic
// so we need to deal with it properly by default
if (val === null || val === undefined) {
return null;
}
const d = new Date(val);
const t = d.getTime(); // to deal with invalid date
return t === t ? d : null;

@atsu85 atsu85 Oct 19, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

is this t === t an optimization compared to !isNaN(t), or why do you use this weird syntax, that only makes sense in JavaScript?

@bigopon bigopon Oct 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes it is used to specifically deal with NaN value. As NaN === NaN is false.

What a spec

@StrahilKazlachev StrahilKazlachev Oct 19, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But does it perform better than using isNaN?

@bigopon bigopon Oct 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thats interesting question. Normally i would say yes but V8 is smart and I believe isNaN could be faster ( from another v8 engineer tweet i saw different between o === o and Object.is(o, o) is 2 us

@atsu85 atsu85 Oct 19, 2017 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@bigopon

As NaN === NaN is false.

Yeah i know, right after figuring that out I started using TsLint ;)

My comment+question was because of readability & preformance. IMHO if t === t is not preforming (much) better than !isNaN(t), then i'd vote for better readability :)

@bigopon bigopon Oct 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see. Cant commit on phone 😁 Will change

@bigopon bigopon Oct 19, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@atsu85 thanks for the suggestion. I just keep doing those things 😄

}
};

Expand All @@ -30,7 +37,7 @@ export const coerceFunctionMap: Map<{new(): any}, string> = new Map([
* Map a class to a string for typescript property coerce
* @param type the property class to register
* @param strType the string that represents class in the lookup
* @param coerceFunction coerce function to register with @param strType
* @param coerceFunction coerce function to register with param strType
*/
export function mapCoerceFunction(type: {new(): any, coerce?: (val: any) => any}, strType: string, coerceFunction: (val: any) => any) {
coerceFunction = coerceFunction || type.coerce;
Expand Down