-
-
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 all 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,51 @@ | ||
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(val) { | ||
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. Since dates are stateful, should there be a check if it's already of type date?
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. This could be nice. I didn't think of it. Maybe we can reuse the instance if this ever gets merged ? @jdanyow / @EisenbergEffect |
||
// 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); | ||
return isNaN(d.getTime()) ? null : d; | ||
} | ||
}; | ||
|
||
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