Skip to content
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

Bindings with specified signal binding behavior (BindingSignaler) are being dirty-checked / not waiting for signals to update the view from view-model getters. #293

Open
m-andrew-albright opened this issue Jul 22, 2017 · 11 comments
Assignees

Comments

@m-andrew-albright
Copy link

I'm submitting a bug report

  • Library Version:
    1.4.0

Please tell us about your environment:

  • Operating System:
    Windows 10

  • Node Version:
    8.2.0

  • NPM Version:
    5.3.0

  • JSPM OR Webpack AND Version
    webpack 2.2.1

  • Browsers:
    | Chrome 59 | Firefox 54

  • Language:
    all

Current behavior:
When binding in the view to a getter property in the view-model and specifying the signal:'my-signal' binding-behavior, the view is streaming the updates instantly.

See the gistrun example here

Expected/desired behavior:

  • What is the expected behavior?
    The getter property should not be accessed until the BindingSignaler signals an update with the specified string.

  • What is the motivation / use case for changing the behavior?
    Unless I misunderstand, the binding signaler is not behaving as it was intended. Some getters (particularly current-time-based computed properties) should only be accessed every so often, so binding signalers seemed to be the best way to do this, except they are not behaving as I would expect.

@m-andrew-albright
Copy link
Author

It appears to have the same behavior whether the binding is string interpolation or property binding (class.bind in case I'm not using the correct term)

@StrahilKazlachev
Copy link
Contributor

StrahilKazlachev commented Jul 22, 2017

The dirty checking is result of timeSince being a property. Put in your example @computedFrom('timeStamp') on timeSince and see if that gives you the behavior you are after.
Also in the docs there is an example for this that uses valueConverter and not a property. I would go with that approach.

@m-andrew-albright
Copy link
Author

Using a value converter attains the expected behavior, but why does using a property getter with a signal binding behavior not work in the same way? I created the repro case on gistrun as a simple example, but I don't necessarily want to use a value converter for every time I want to throttle a computed property value.

@m-andrew-albright
Copy link
Author

After some digging (and some memory surfaced) I found this:

aurelia/binding#547

The important piece:

Aurelia observes computed props (props with a getter or setter function) using dirty-checking. The dirty-checking interval is a system-wide setting, 120ms is the default.

The throttle and debounce binding behaviors to not change the way properties are observed, they change the way the binding responds to the property changes. Instead of responding to the property change immediately, the binding's response will be throttled/debounced.

which leads me to wonder, is there some way I can tell Aurelia not to dirty check a computed property? It would be great (IMO) if it could detect that all of its bindings have a binding behavior which specify that it shouldn't be observed, so the dirty checking is not necessary. I don't know how feasible that would be.

@StrahilKazlachev
Copy link
Contributor

If you don't want something to be observed use .one-time/& oneTime. If you put & oneTime on all your bindings in the example then the property will not be observed, in this case dirty checked.

@m-andrew-albright
Copy link
Author

I want the view to update when the property is updated / signaled to be updated, so oneTime wouldn't make sense here. If I am binding with a signaler, I would hope the property is not being observed / dirty-checked since its only binding already signals when it should update.

I have come to understand that this is not the way that it works, but IMO it should.

@Alexander-Taran
Copy link

well docs state it like:

The signal binding behavior enables you to "signal" the binding to refresh. This is especially useful when a binding result is impacted by global changes that are outside of the observation path.

in other words, it augments binding with a possibility to force reevaluation
usual path of observation still applies.

@Alexander-Taran
Copy link

Alexander-Taran commented Feb 26, 2018

maybe can be closed

@mattgaspar
Copy link

oneTime only applies to the initial binding behavior. If you add a signal it will still work as desired in this issue.
I'm not sure if order is important but I always include the oneTime behavior before the signal.

repeat.for="[i, comment] of comments & oneTime & signal:'comment-added'"

@Esger
Copy link

Esger commented Feb 17, 2021

@mattgaspar In a binding like this <img width.bind="portraitChannel | slideThumbWidth:{smallThumb:smallThumb} & oneTime & signal:'thumb-size-changed'"> that doesn't seem to be the case.
The one-time overrules the signal, no matter the order, or with string interpolation i.s.o. .bind

@bigopon
Copy link
Member

bigopon commented Feb 17, 2021

I don't think we want the signal binding behavior automatically assumes that it's in charge of the binding update by default. It should be treated as an additional way to notify a binding to update.

With that basic in mind, we can refactor the behavior a little bit, so that if the first parameter is an array, then it's assume it's this overload:

interface SignalBindingBehavior {
  bind(binding: Binding, source: Scope, names: string[], controlBinding?: boolean);
}

Note the controlBinding flag, if true, it will change binding mode of the binding to one time, and do some extra necessary stuff so that the binding will not dirty check.

This is quite an involved refactoring, as it probably requires some changes in binding module as well, I think we may not want to add support for it in v1. In v2, this may be easier to support, though we probably want to wait a bit.

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

No branches or pull requests

8 participants