feat(ValueConverter): enable signal

[bigopon]

This PR:

• Addresses Value converters should support triggering updates #353 Value converters should support triggering updates
• Has potential integration with BindingSignaler in templating-resources, where signaling in there will also trigger here, treating signal universal
• Still needs tests (WIP)
• Usage:

export class LocaleValueConverter {
  signals: string | string[] = 'locale-changed'
}

// usage
import { ConverterSignaler } from 'aurelia-binding';
ConverterSignaler.signal('some-thing');

• Demo
• Updated Demo

@jdanyow @EisenbergEffect @thomas-darling @davismj

[jdanyow]

It would be nice if we could unify this API with the existing binding signaler API. I think there's an opportunity here to tie signaling in at a lower level, in the "connect" phase and refactor the existing signaling logic to leverage the existing binding machinery related to property observation.

Today, the ValueConverter expression class looks like this:

export class ValueConverter extends Expression {
  ...
  ... other methods ...
  ...
  connect(binding, scope) {
    let expressions = this.allArgs;
    let i = expressions.length;
    while (i--) {
      expressions[i].connect(binding, scope);
    }
  }
}

I think we could change it to something like this:

import { connectBindingToSignal } from './signals';

export class ValueConverter extends Expression {
  ...
  ...
  ...
  connect(binding, scope) {
    const expressions = this.allArgs;
    let i = expressions.length;
    while (i--) {
      expressions[i].connect(binding, scope);
    }
    // new stuff:
    const signals = binding.lookupFunctions.valueConverters(this.name).signals;
    if (signals === undefined) {
      return;
    }
    i = signals.length;
    while (i--) {
      connectBindingToSignal(binding, signals[i]);
    }
  }
}

aurelia-binding - signals.js (new module)

const signals = Object.create(null);

export function connectBindingToSignal(binding, name) {
  if (!signals.hasOwnProperty(name)) {
    signals[name] = 0;
  }
  binding.observeProperty(signals, name);
}

export function signalBindings(name) {
  if (signals.hasOwnProperty(name)) {
    signals[name]++;
  }
}

Then our BindingSignaler class in templating-binding would reuse the new signalBindings function. We should probably mark it obsolete and inform users to switch over to signalBindings.

The signal binding behavior would become much simpler by reusing then new connectBindingToSignal function.

What do you think?

[bigopon]

You meant we determine signals based on BindingBehavior / ValueConverter -> signals properties ?
Edit: i got what you meant. Sounds better to me. Will change / push another PR in templating resources

[bigopon]

@jdanyow Things looks a lot neater now. very cool 👍 💯

• templating-resources also ready.
• What do you think about the tests ?

[jdanyow]

tests look good, nice work.

[jdanyow]

This is a really hot path. Let's remove this line entirely to ensure the connect overhead remains minimal.

I don't think supporting string & array is necessary, just more to document & maintain. If we were to do that here's a good spot to perform the normalization, but again, I would prefer to keep the API surface small and strict:

binding/src/value-converter-resource.js

Lines 15 to 17 in b6565eb

	initialize(container, target) {
	this.instance = container.get(target);
	}

[bigopon]

@jdanyow All done as suggested. Learnt to be more careful with connect. About the API, 99% of usage will be a single signal, I do think we need to support normalisation, but I'll let you make the call after this got merged. What do you think ?

[EisenbergEffect]

@bigopon Would you be willing to PR some documentation for this feature in the value converter doc?

…

Sent from my iPhone

On Oct 8, 2017, at 9:01 AM, Jeremy Danyow ***@***.***> wrote: Merged #626. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[bigopon]

Sure i enjoy doing it, and discussing with Jeremy too 😁

…

On 9 Oct. 2017 3:30 am, "Rob Eisenberg" ***@***.***> wrote: @bigopon Would you be willing to PR some documentation for this feature in the value converter doc? Sent from my iPhone > On Oct 8, 2017, at 9:01 AM, Jeremy Danyow ***@***.***> wrote: > > Merged #626. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub, or mute the thread. > — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#626 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJiBIV8reia9y29UO8qTthL5DJxV46zWks5sqPiigaJpZM4PqJoF> .