Skip to content

Conversation

@bigopon
Copy link
Member

@bigopon bigopon commented Jul 31, 2017

This PR add support for coercing value, before calling setValue on property, to address the issue: #96

Update:

  • Should import coerceFunctions from 'aurelia-binding'
  • Should have docs
  • Should update typescript definition

Usage is in observable PR

Details

Usage is similar to @jdanyow 's proposal

class MyViewModel {
  @bindable({ coerce: 'number' }) numProp
  @bindable({ coerce: 'boolean' }) boolProp
  @bindable({ coerce: val => convertValue(val) }) customCoerceProp
}

To modify / extends the built-in coerce types:

import {coerces} from 'aurelia-templating';

coerces.string = function(a) {
  return a === null || a === undefined ? '' : a.toString();
}

To add more coerce types:

coerces.longDate = function(a) {
  return new Date(a).toJSON();
}

// usage:
@bindable({ name: 'date', coerce: 'longDate' })
class DateControl {

}

All basic implementations:

const numCons = Number;
const dateCons = Date;
const _isFinite = isFinite;
const _isNaN = isNaN;

export const coerces = {
  none(a) {
    return a;
  },
  number(a) {
    var val = numCons(a);
    return !_isNaN(val) && _isFinite(val) ? val : 0;
  },
  string(a) {
    return '' + a;
  },
  boolean(a) {
    return !!a;
  },
  date(a) {
    return new dateCons(a);
  }
};

To avoid breaking changes/ bug, any fail attempt to get coerce function will result into a identity function, which is coerces.none

cc @EisenbergEffect @jdanyow

@bigopon
Copy link
Member Author

bigopon commented Jul 31, 2017

@EisenbergEffect if this passes, it will be trivial to support your proposal:

class MyViewModel {
  @bindable.number numProp
}

@EisenbergEffect
Copy link
Contributor

Cool. The other thing we'd want to do is support TypeScript metadata, so if they do this...

number: boolean;

...we should pick up the metadata and map it to a coerces function as well.

@bigopon
Copy link
Member Author

bigopon commented Aug 1, 2017

I have added the support for Typescript.

Now it supports this:

import {mapCoerceForClass} from 'aurelia-templating';

class Point {}

mapCoerceForClass(Point, 'point', function(value) {
  return new Point(value.split(' ').map(parseFloat));
});

class LineViewModel {
  point1: Point
  point2: Point
}

Basically you can register how to coerce a class. JavaScript users can also leverage this by setting metadata on property:

import {mapCoerceForClass} from 'aurelia-templating';

class Point {}

mapCoerceForClass(Point, 'point', function(value) {
  return new Point(value.split(' ').map(parseFloat));
});

class LineViewModel {

  @bindable
  @Reflect.metadata('design:type', Point)
  point1

  @bindable
  @Reflect.metadata('design:type', Point)
  point2
}

@EisenbergEffect one thing I'm not sure & confident about is naming of mapCoerceForClass & classCoerceMap (this is a map holding Class definition and their string representation for look up in coerces export)

Code for classCoerceMap:

/**@type {Map<Function, string>} */
export const classCoerceMap = new Map([
  [Number, 'number'],
  [String, 'string'],
  [Boolean, 'boolean'],
  [Date, 'date']
]);

/**
 * Map a class to a string for typescript property coerce
 * @param Class {Function} the property class to register
 * @param strType {string} the string that represents class in the lookup
 * @param converter {function(val)} coerce function tobe registered with @param strType
 */
export function mapCoerceForClass(Class, strType, coerce) {
  if (typeof strType !== 'string' || typeof coerce !== 'function') {
    LogManager.warn(`Bad attempt at mapping class: ${Class.name} to type: ${strType}`);
  }
  coerces[strType] = coerce;
  coerceClassMap.set(Class, strType);
}

It can be even more convenient, if in function mapCoerceForClass, we look for static method with name coerce so user can define class like this:

class Point {
  static coerce(value) {
    return new Point(value.split(' ').map(parseFloat));
  }
}

// Usage:
mapCoerceForClass(Point, 'point');

If static coerce method is more desired, then probably the method mapCoerceForClass can be refactored into 2 ways of usage: decorator and normal function call. Example:

// Decorator usage
@mapCoerceForClass('point')
class Point {
  static coerce(value) {
    return new Point(value.split(' ').map(parseFloat));
  }
}

// Or normal usage:
mapCoerceForClass(Point, 'point');

@bigopon
Copy link
Member Author

bigopon commented Aug 1, 2017

I just re-read the issue again and it seems you @EisenbergEffect agreed with @niieani that it shouldn't use metadata to automatically coerce value. So I'm not actually sure about the changes related to the metadata.

Update: sorry I misread it. time for a nap i guess 😢

@EisenbergEffect
Copy link
Contributor

Don't worry too much about the api yet. We can tweak that. If you'd like to keep working on this (awesome work btw!) then I'm wondering if you can investigate the binding library's observable decorator. I can imagine wanting to support this with both observable and bindable, so I'm wondering if we can put the code in the binding library somehow, or at least the core of it that is independent of the decorators.

const numCons = Number;
const dateCons = Date;
const _isFinite = isFinite;
const _isNaN = isNaN;
Copy link

Choose a reason for hiding this comment

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

@bigopon, could you explain the purpose of these 4 aliases - it looks like adding indirection that makes readability worse without obvious benefits

Copy link
Member Author

@bigopon bigopon Aug 2, 2017

Choose a reason for hiding this comment

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

It is expected that those functions will be called with high frequency, so it is better to put them in the same scope with the calling function. This is to work around bundlers, which will add wrap those functions in many scope layers containing maybe up to hundred of enumerable accessors.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's skip this for now and re-visit if we see an issue with bundlers. Makes the code less readable.

};

/**@type {Map<Function, string>} */
export const classCoerceMap = new Map([
Copy link

@atsu85 atsu85 Aug 2, 2017

Choose a reason for hiding this comment

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

I think that instead of the two lines above You can write smth like:
export const classCoerceMap: Map<Function, string> = new Map([
or even better (more precise type):
export const classCoerceMap: Map<(strValue: string) => any, string> = new Map([
as it would be correct way to write type information for TypeScript variables/constants, and at the moment some Babel plugin is used to extract this information to generate API docs and definitions files for TS.

My only concern is that maybe the Babel plugin that extracts TS types doesn't understand so complex TS type expressions as i wrote in the second example

Copy link

Choose a reason for hiding this comment

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

This would also be beneficial when converting this repo to TS (as i understand, some Aurelia repos are already written in TypeScript and some are in middle of transitioning to TS)

Copy link
Member Author

@bigopon bigopon Aug 2, 2017

Choose a reason for hiding this comment

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

You are right about this, could have inlined type def.
The purpose of this variable is to hold a map between Classes and their coerce type (as string) in real look up table of coerces for later use. In other word, string type is the source of truth. So the type would be Map<Function, string>

* @param strType {string} the string that represents class in the lookup
* @param converter {function(val)} coerce function tobe registered with @param strType
*/
export function mapCoerceForClass(Class, strType, coerce) {
Copy link

Choose a reason for hiding this comment

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

perhaps type information could be moved next to parameters from function documentation - smth like
export function mapCoerceForClass(Class: Function, strType: string, coerce: (strValue: string) => any) {

@atsu85
Copy link

atsu85 commented Aug 2, 2017

@bigopon, nice work! I added couple of comments tough

@EisenbergEffect
Copy link
Contributor

@jdanyow Can you work with @bigopon to address any issues and add support for this for both observable and bindable? This is a long time requested feature and one I know people will want to use.

this.publishing = false;
this.selfSubscriber = selfSubscriber;
this.currentValue = this.oldValue = initialValue;
if (typeof coerce !== 'undefined') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (coerce !== undefined) { (rest of codebase doesn't care about crazy people remapping undefined. 😉 )

const numCons = Number;
const dateCons = Date;
const _isFinite = isFinite;
const _isNaN = isNaN;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's skip this for now and re-visit if we see an issue with bundlers. Makes the code less readable.

return a;
},
number(a) {
var val = numCons(a);
Copy link
Contributor

Choose a reason for hiding this comment

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

const

* Map a class to a string for typescript property coerce
* @param Class {Function} the property class to register
* @param strType {string} the string that represents class in the lookup
* @param converter {function(val)} coerce function tobe registered with @param strType
Copy link
Contributor

Choose a reason for hiding this comment

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

tobe -> to

*/
setValue(newValue: any): void {
let oldValue = this.currentValue;
let realNewValue = this.coerce ? this.coerce(newValue) : newValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

const coercedValue = this.coerce === undefined ? newValue : this.coerce(newValue);

const _isFinite = isFinite;
const _isNaN = isNaN;

export const coerces = {
Copy link
Contributor

Choose a reason for hiding this comment

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

coercionFunctions ?

};

/**@type {Map<Function, string>} */
export const classCoerceMap = new Map([
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe name coercionFunctionMap?

* @param strType {string} the string that represents class in the lookup
* @param converter {function(val)} coerce function tobe registered with @param strType
*/
export function mapCoerceForClass(Class, strType, coerce) {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about:

mapCoercionFunction(type: { new:(): any; }, typeName: string, coercionFunction: (value: string) => any)

if (propType) {
nameOrConfigOrTarget = classCoerceMap.get(propType) || 'none';
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this turning on coercion by default in any project that uses TypeScript with emit decorator metadata?

cc @EisenbergEffect

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to have a setting that allows people to opt into this. Turning it on automatically, at this point, would be a breaking change to people's apps.

@bigopon
Copy link
Member Author

bigopon commented Sep 5, 2017

@jdanyow @EisenbergEffect

I have:

  • fixed all naming issues
  • added a flag to turn off the type auto pickup by default. Also export a function to turn it on
  • added a function createTypedBindable to create bindable with fluent syntax @bindable.string.
  • created built in for string, number, date, boolean with createTypedBindable

Please let me know if I missed something.

Copy link
Contributor

@jdanyow jdanyow left a comment

Choose a reason for hiding this comment

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

This is looking good... couple minor feedback items. The other thing we'll need are docs and updates to aurelia-binding.d.ts interfaces (unlike the other repos this one is hand written).

setValue(newValue: any): void {
let oldValue = this.currentValue;
const oldValue = this.currentValue;
const coercedValue = this.coerce !== undefined ? this.coerce(newValue) : newValue;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick- positive case first please. this.coerce === undefined ? newValue : ....

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

case 'string':
c = coerceFunctions[coerce]; break;
default: break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit pick- does the indentation of the switch block pass the lint rules?

Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't see it in .eslint.json in aurelia-tools. When I was doing this, I remembered I got error in linting for not indenting the case. And I tested with 1 more indent, it showed error. I think it's following the rule.

default: break;
}
if (c === undefined) {
LogManager
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be put in the default: switch case? Then move the this.coerce assignment into the function and string cases, enabling removal of the let c?

@bigopon
Copy link
Member Author

bigopon commented Sep 25, 2017

@jdanyow

I have added typings for bindable, BindableProperty. bindable.string, ...etc.

I haven't added typings for coerceFunctions and mapCoerceFunction because it would be more desirable to have both @observable and @bindable to have same sort of functionality (per @EisenbergEffect ). So I think we will need to move coerceFunctions and mapCoerceFunction there. There was a PR in binding for that but I messed up my history and it's gone. Can easily recreate though.

I don't know if this is the final desired behavior, as currently it still doesn't play nice together with two-way binding. Basically it can't be synced well with input, for current binding notification model. And it would take not trivial amount work to support it properly, consider syncing between a date bindable property to an input, or select. We can, however, encourage user to use from-view and typed bindable.

What do you think ?

Edit: observable pr adds the base for coerceFunctions for this PR.

@bigopon bigopon changed the title feat(bindable): support coercing value WIP feat(bindable): support coercing value Oct 12, 2017
@bigopon
Copy link
Member Author

bigopon commented Oct 21, 2017

@jdanyow @EisenbergEffect doc is ready too. oh docs ...

bindable and observable now have same usage. except you have to enable auto property type pick up in each of them:

observable.usePropertyType(true);
bindable.usePropertyType(true);

Maybe a FrameworkConfiguration method should be added for this ?

Copy link

@atsu85 atsu85 left a comment

Choose a reason for hiding this comment

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

Fantastic work @bigopon. I added few comments tough :)


#### Usage with metadata for Typescript

Typescript compiler has an option emit class fields with their types in metadata. `bindable` decorator can work with this via `usePropertyType` function, which is a property of `bindable` decorator:
Copy link

Choose a reason for hiding this comment

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

Perhaps instead of "an option emit" you meant "an option to emit"?


<code-listing heading="extend-coerce">
<source-code lang="TypeScript">
// import from 'aurelia-binding' if you are writing a plugin
Copy link

Choose a reason for hiding this comment

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

is it explained somewhere else, why plugins should import things from original package instead of aurelia-framework that exports things from other packages? I guess they both work, but 'aurelia-binding' should be preferred to avoid excessive dependencies (and to be tree-shaking friendly)?


#### Fluent syntax

If you scroll back top a bit, you will notice we had fluent syntax decorator: `@bindable.number`. This, as described above, is a simplified and more expressive form of `coerce: 'number'`. Aurelia also provides a way to build your own fluent syntax `bindable` decorator. You can do this by:
Copy link

Choose a reason for hiding this comment

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

Perhaps instead of
"If you scroll back top a bit"
following would be better:
"If you scroll back up a bit"

// Add `point` coerce function
coerceFunctions.point = function(incomingValue) {
return incomingValue.split(' ').map(parseFloat);
};
Copy link

Choose a reason for hiding this comment

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

Perhapse there should be a helper to simplify registering fluent coerce syntax? For example instead of

  • import {coerceFunctions, coerceFunctionMap, createTypedBindable} from 'aurelia-framework';
  • createTypedBindable(..)
  • coerceFunctionMap.set(...)
  • coerceFunctions.... = function...
    it could be done for example like this:
import {coerceHelper} from 'aurelia-framework';
coerceHelper.addFuluentBindableExtension('point', Point, (incomingValue) => {
    return incomingValue.split(' ').map(parseFloat);
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I was feeling this is too much call too, for API designed, I think @jdanyow @EisenbergEffect will need to give call on this.

@atsu85 Thanks for helping, I ran out of energy half way through the docs 😱 , many typos and unneeded stuff

@EisenbergEffect
Copy link
Contributor

@bigopon Can you give me an update on where we are at on this. Seems like it stalled out. I'd love to have types bindables/observables though.

@thomas-darling
Copy link

This looks really awesome, @bigopon - great work, and much appreciated!

I do have a couple of concerns though:

  • The use with TypeScript seem too complicated. If the property type is number, and the value being set is a string, it should just always coerce by default - having to explicitly turn that on for every single bindable property will be a huge pain, and it would make no sense, as you already specified that the desired type, and the compiler will complain if you try to treat it as anything else.

    Not coercing by default will just cause bugs, exactly as it does today, where the compiler and developer thinks a property will be of the type specified, but then at runtime, it ends up being something completely different. In our code, this is probably our single biggest source of bugs.

  • As I understand it, boolean attributes such as disabled are not handled automatically, but requires a coerce function to be specified. I think handling boolean attributes according to the HTML spec should be the default behavior.

    Previously, we also talked about having a reflect option on the bindable decorator, which would control whether the property value is reflected to the DOM attribute or not - I think that’s also still relevant.

  • The concept of coerce functions do not support two way binding, as far as I can tell, and I’m wondering whether we need this new concept at all? We already have value converters, which essentially do the same thing, and support two-way conversion - maybe we should just allow those to be used, instead of introducing a new concept?

    The only issue with that, would be how to support boolean attributes, where they have to not just convert the value, but also modify the attribute on the element. However, since boolean attributes are really a very special case, I think it might be ok to just hardcore support for that...

    So if that reflect option I mentioned is set to true, and toView returns true, then the attribute is automatically added with an empty string as value. If toView returns false, null or undefined, then the attribute is automatically removed. Maybe with an exception for attributes whose name starts with aria-, as those do not follow the standard behavior, but requires the values “yes” and “no”.

    It’s a simple solution, but it gets the job done, and it would also solve previously reported issues with attributes, such as src and href, which should only be present if they actually have a value.

    An alternative approach might be to, when the value converter is used for coercion and that reflect option is true, pass the element and attribute name to the toView and fromView functions of the converter. Then the converter could make the nessesary attribute changes - and again, if we choose this approach, then I think the boolean converter we provide out of the box should do this by default.

Now, we might need options to opt-out of some of this default behavior until app code is updated, but I think that will be the best long-term strategy, as opposed to littering the app code with options to opt-in on every bindable property.

Thoughts, @bigopon, @EisenbergEffect?

@bigopon
Copy link
Member Author

bigopon commented Dec 11, 2017

@thomas-darling

The use with TypeScript seem too complicated. If the property type is number ...

Are you referring to bindable.usePropertyType(true) ? If that is true, then it shouldn't be a concern as it's supposed to be called only once. It is there for compatibility reason.

As I understand it, boolean attributes such as disabled are not handled automatically ...

View model properties are not supposed to be used with view only, but also interacted within JavaScript / TypeScript. so by default I don't think we can treat @bindable.boolean similar to Html boolean attribute. Consider "", it's true for Html boolean attributes but false for Js/Ts boolean. I think best we can do is to have another default: @bindable.booleanAttr where it treat "", or "required", "readonly" as true

The concept of coerce functions do not support two way binding, as far as I can tell, and I’m wondering whether we need this new concept at all? ...

It could be nice if we can avoid it, but for me, value converter are somewhat view related logic, while typed bindable/ observable are supposed to work seamlessly in script side. Beside that, it would bring huge overhead to make current value converter work within Ts/Js I think. @EisenbergEffect & @jdanyow can give better feedback

Previously, we also talked about having a reflect option on the bindable decorator ...

It's is being discussed here #560 . Ideally we should have it reviewed after this PR is decided to be merged or not

The concept of coerce functions do not support two way binding ...

It depends on the target of the binding, if it's a custom element / custom attribute, then it works well and as expected, except some cases like gotchas in the observable PR. For Html built-ins, value are always coerced to string so it's pretty much nothing we can do.

@atsu85
Copy link

atsu85 commented Dec 11, 2017

@EisenbergEffect, @jdanyow - i've been really dreaming about this feature since the beginning. Contrary to what @thomas-darling said, this feature is specially useful and easy to use with TypeScript, as reflect metadata is available at runtime and the fact that we'd need to call bindable.usePropertyType(true) just once during Aurelia app configuration doesn't make absolutely any difference for me.

Fantastic work @bigopon, i hope it will be merged soon!

@CasiOo
Copy link

CasiOo commented Dec 12, 2017

I have some refactor suggestions, getting inspiration from the validation plugin.
In validation we're using static methods and properties to add custom rules without having an instance. Maybe we should be doing the same here?
ValidationRules.customRule(...);
https://github.com/aurelia/validation/blob/master/src/implementation/validation-rules.ts#L463

So here are my suggestions

// Add fluent binding syntax for coercion converter by default.
// Optionally state a property type. Useful for TS users or JS using reflection metadata.
Coercion.customConverter(key, converter, targetPropertyType?);

// Interpret of property types should be set on a Coercion object and not on bindable in my opinion.
Coercion.usePropertyType(true);
// or
Coercion.interpretPropertyType(true);

<source-code lang="ES 2015">
export class NumberField {
@bindable label
@bindable({ coerce: val => Number(val) }) value
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be @bindable({ coerce: Number }) value or is that confusing?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks I'll fix it the next commit 👍

@bigopon
Copy link
Member Author

bigopon commented Mar 31, 2018

Since observable PR got closed, probably we will also close this PR.

For anyone interested in this feature:

https://github.com/bigopon/aurelia-typed-observable-plugin

@bigopon bigopon closed this Mar 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants