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

Value converters should support triggering updates #353

Closed
thomas-darling opened this Issue Mar 21, 2016 · 8 comments

Comments

Projects
None yet
5 participants
@thomas-darling

thomas-darling commented Mar 21, 2016

Ok, so here we go...

Background

I understand the concept of binding behaviors, and think they are a really great idea for things like throttling binding updates - for that stuff they make a lot of sense. However, the more I work with this, the more I find myself wrapping a value converter in a binding behavior, just to make sure it updates when the user changes site settings such as locale or currency.

The TBindingBehavior in the Aurelia i18n library is a perfect example of this:
https://github.com/aurelia/i18n/blob/master/src/t.js

Now, we are a large, global site and unfortunately we found the Aurelia i18n library to be quite insufficient for our needs - that's a topic for another discussion that I would love to have with you guys, but the point is that we are working on a custom library for this, and in doing that, we need to implement a number of custom value converters to handle formatting of things like currencies, dates, times, numbers, percentages, pluralization, lists, prepositions, word variants, and lots of other crazy stuff.

The problem is, every single one of those converters need to be wrapped in a corresponding binding behavior, as it needs to update whenever the locale or currency is changed. Actually, I can't think of very many uses for value converters where such a requirement would not be present - after all, their fundamental purpose is to convert values to/from their view representation, and that will almost always be a localized string.

I feel like this workaround is an abuse of the binding behavior concept, as what we're doing here feels much more like a value conversion than like a behavioral change. And if every converter has to be wrapped in a behavior anyway, then why even have the distinction? It just blurs the line between the two concepts, and ultimately makes it a bit of a mess.

Proposal

The way I see it, binding behaviors should be used only for things like throttling binding updates or setting things like oneWay or oneTime - uses where they actually modify the fundamental behavior of the binding. Meanwhile, value converters should have a way to signal to the underlying binding that it needs to update - it does not need to have full access to the actual binding instance, it just needs some way to signal to the binding that it needs to update... or maybe even simpler, just a property that we can specify, which lists the signals that should trigger an update (you can think of it as just stating dependencies on signals, just like the constructor states dependencies on services, etc.)

Example:

export class CurrencyValueConverter
{
    // When the framework instantiates the binding converter, it should look for this property,
    // and if present, it should subscribe the binding to the specified signals.
    signals = ['currency-changed', 'locale-changed'];
    ...
}

This would make this whole thing so much nicer and more intuitive to work with, and I think it would make the distinction between the two concepts much more clear.

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Mar 21, 2016

Member

@jdanyow What are your thoughts? This could certainly be handy...

Member

EisenbergEffect commented Mar 21, 2016

@jdanyow What are your thoughts? This could certainly be handy...

@jdanyow

This comment has been minimized.

Show comment
Hide comment
@jdanyow

jdanyow Mar 21, 2016

Member

I like the idea of associating signal names with value converters. It may not be as "pure" of an approach as passing the necessary data to the value converter as arguments but it sure seems like it would make localization work a lot more terse than ${value | currency:currentLocale} or ${value | currency & signal:'locale-changed'}.

Should the signal names array be static?

@signals('currency-changed', 'locale-changed')
export class CurrencyValueConverter {
}
export class CurrencyValueConverter {
  static signals= ['currency-changed', 'locale-changed'];
}

Not sure if that matters because converters are singletons. Thoughts?

Member

jdanyow commented Mar 21, 2016

I like the idea of associating signal names with value converters. It may not be as "pure" of an approach as passing the necessary data to the value converter as arguments but it sure seems like it would make localization work a lot more terse than ${value | currency:currentLocale} or ${value | currency & signal:'locale-changed'}.

Should the signal names array be static?

@signals('currency-changed', 'locale-changed')
export class CurrencyValueConverter {
}
export class CurrencyValueConverter {
  static signals= ['currency-changed', 'locale-changed'];
}

Not sure if that matters because converters are singletons. Thoughts?

@thomas-darling

This comment has been minimized.

Show comment
Hide comment
@thomas-darling

thomas-darling Mar 21, 2016

Awesome! Based on the scenarios I've encountered thus far, a static attribute like that will be perfect :-)

thomas-darling commented Mar 21, 2016

Awesome! Based on the scenarios I've encountered thus far, a static attribute like that will be perfect :-)

@jdanyow jdanyow added the enhancement label Mar 21, 2016

@EisenbergEffect

This comment has been minimized.

Show comment
Hide comment
@EisenbergEffect

EisenbergEffect Mar 22, 2016

Member

They are singletons so an instance member should work I think.
On Mar 21, 2016 6:43 PM, "Thomas Darling" notifications@github.com wrote:

Awesome! Based on the scenarios I've encountered thus far, a static
attribute like that will be perfect :-)


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#353 (comment)

Member

EisenbergEffect commented Mar 22, 2016

They are singletons so an instance member should work I think.
On Mar 21, 2016 6:43 PM, "Thomas Darling" notifications@github.com wrote:

Awesome! Based on the scenarios I've encountered thus far, a static
attribute like that will be perfect :-)


You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#353 (comment)

@thomas-darling

This comment has been minimized.

Show comment
Hide comment
@thomas-darling

thomas-darling May 24, 2016

Any update on this? We're really looking forward to it :-)

thomas-darling commented May 24, 2016

Any update on this? We're really looking forward to it :-)

@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Jun 2, 2016

Member

I'd like to see this as well. The problem I keep running into is that a binding does not subscribe to the data that the ValueConverter is processing. In my newest scenario, I want to convert an array of coordinate pairs into an SVG path's d attribute. Because this is an attribute binding, neither adding new coordinates to the array nor changing existing coordinates in the array update the binding. See below:

export class ToSVGPathValueConverter {
    toView(path: IPosition[]): string {
        return 'M' + path
            .map((pos) => `${pos.x} ${-pos.y}`)
            .join(' L ');
    }
}

We definitely need some sort of mechanism for fine tuning control over when a ValueConverter recomputes.

Pie in the sky answer is to declare observed properties implicitly or explicitly on the ValueConverter. Implicitly meaning "it just works" knockout style. Explicitly meaning the following:

@computedFrom('path.length', 'path[].x', 'path[].y')
export class ToSVGPathValueConverter {
    toView(path: IPosition[]): string {
        return 'M' + path
            .map((pos) => `${pos.x} ${-pos.y}`)
            .join(' L ');
    }
}
Member

davismj commented Jun 2, 2016

I'd like to see this as well. The problem I keep running into is that a binding does not subscribe to the data that the ValueConverter is processing. In my newest scenario, I want to convert an array of coordinate pairs into an SVG path's d attribute. Because this is an attribute binding, neither adding new coordinates to the array nor changing existing coordinates in the array update the binding. See below:

export class ToSVGPathValueConverter {
    toView(path: IPosition[]): string {
        return 'M' + path
            .map((pos) => `${pos.x} ${-pos.y}`)
            .join(' L ');
    }
}

We definitely need some sort of mechanism for fine tuning control over when a ValueConverter recomputes.

Pie in the sky answer is to declare observed properties implicitly or explicitly on the ValueConverter. Implicitly meaning "it just works" knockout style. Explicitly meaning the following:

@computedFrom('path.length', 'path[].x', 'path[].y')
export class ToSVGPathValueConverter {
    toView(path: IPosition[]): string {
        return 'M' + path
            .map((pos) => `${pos.x} ${-pos.y}`)
            .join(' L ');
    }
}
@davismj

This comment has been minimized.

Show comment
Hide comment
@davismj

davismj Oct 10, 2017

Member

Nice work, @bigopon

Member

davismj commented Oct 10, 2017

Nice work, @bigopon

@bigopon

This comment has been minimized.

Show comment
Hide comment
@bigopon

bigopon Oct 10, 2017

Member

Thanks 😄 @davismj

Member

bigopon commented Oct 10, 2017

Thanks 😄 @davismj

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment