-
-
Notifications
You must be signed in to change notification settings - Fork 96
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.
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
feat(observable): coercion, typed observable #623
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
import * as LogManager from 'aurelia-logging'; | ||
|
||
export const coerceFunctions = { | ||
none(a) { | ||
return a; | ||
}, | ||
number(a) { | ||
const val = Number(a); | ||
return !isNaN(val) && isFinite(val) ? val : 0; | ||
}, | ||
string(a) { | ||
return '' + a; | ||
}, | ||
boolean(a) { | ||
return !!a; | ||
}, | ||
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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes it is used to specifically deal with NaN value. As What a spec There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But does it perform better than using There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah i know, right after figuring that out I started using TsLint ;) My comment+question was because of readability & preformance. IMHO if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see. Cant commit on phone 😁 Will change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @atsu85 thanks for the suggestion. I just keep doing those things 😄 |
||
} | ||
}; | ||
|
||
export const coerceFunctionMap: Map<{new(): any}, string> = new Map([ | ||
[Number, 'number'], | ||
[String, 'string'], | ||
[Boolean, 'boolean'], | ||
[Date, 'date'] | ||
]); | ||
|
||
/** | ||
* 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 | ||
*/ | ||
export function mapCoerceFunction(type: {new(): any, coerce?: (val: any) => any}, strType: string, coerceFunction: (val: any) => any) { | ||
coerceFunction = coerceFunction || type.coerce; | ||
if (typeof strType !== 'string' || typeof coerceFunction !== 'function') { | ||
LogManager | ||
.getLogger('map-coerce-function') | ||
.warn(`Bad attempt at mapping coerce function for type: ${type.name} to: ${strType}`); | ||
return; | ||
} | ||
coerceFunctions[strType] = coerceFunction; | ||
coerceFunctionMap.set(type, strType); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect either NaN or Infinity to be returned if not a valid number, not 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NaN
doesn't work nicely with===
(two way binding scenarios), to handle it we need to sacrifice perf for more checks. With hundreds of bindings perf hit will be worse. I'm not sure what to do here.Infinity
, on the other hand, still problematic but could be treated as is.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it is a problem with two-way binding. Still, as a developer I would not expect 0 to be returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it could be application specific where
NaN
/Infinity
are expected. For that case, we can always define another decorator viacreateTypedObservable
:// usage