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

Bug using "with binding", get "You cannot apply bindings multiple times to the same element" error message #2285

Closed
epabon opened this issue Aug 29, 2017 · 15 comments

Comments

@epabon
Copy link

epabon commented Aug 29, 2017

<div  data-bind="with: $root.getData.bind($data)">
    <div>
        <div data-bind="foreach: $root.items.sort(function (l, r) { return l.Value > r.Value ? 1 : -1 })">
            <div data-bind="text: Name"></div>
        </div>
    </div>
    <div data-bind="style: { 'color': Levels[$data].BackgroundColor }, text: Levels[$data].Name"></div>
</div>

In the view model:

self.getData = function () {
        var value = 0;
        var count = 0;
        for (var i = 0, length = this.Children().length; i < length; i++) {
            var child = this.Children()[i];
            value += child.Level.Value();
            count++;
        }
        return count == 0 ? 0 : Math.ceil(value / count);
}

This return the error message "You cannot apply bindings multiple times to the same element" when a Level Value changes. If I change the binding to: data-bind="with: $root.getData($data)" the error disappears but when the getData return 0 the binging change it to null or undefined.

@epabon
Copy link
Author

epabon commented Aug 30, 2017

You can reproduce this error here: http://jsfiddle.net/epabon/g15jphta/4/

@mbest
Copy link
Member

mbest commented Aug 30, 2017

Thanks for the detailed information. It looks like this was introduced in 3.4.1 because your example works correctly in 3.4.0.

@epabon
Copy link
Author

epabon commented Aug 30, 2017

Yes, It was working correctly in 3.4.0 but I updated to 3.4.1 and this bug appeared

@brianmhunt
Copy link
Member

brianmhunt commented Sep 1, 2017

Here's a more concise example: http://jsfiddle.net/g15jphta/6/

The issue was that the valueAccessor() needed to be called (if it's a function) to create the dependency. I believe I have the right fix for it in TKO (basically if (typeof valueAccessor() === 'function') {valueAccessor()()} in the render loop), but the TKO fix doesn't back-port/translate directly since it's using the new binding class style.

brianmhunt added a commit to knockout/tko that referenced this issue Sep 1, 2017
@mbest mbest closed this as completed in 2606678 Oct 1, 2017
@mbest
Copy link
Member

mbest commented Oct 1, 2017

With the changes in #2234, functions passed to the with binding weren't being called. I've checked in a change to fix that, and it also fixes the above problem.

@miellaby
Copy link
Contributor

miellaby commented Oct 1, 2017

why am I wrong prefering the new behavior?

@mbest
Copy link
Member

mbest commented Oct 1, 2017

@miellaby Can you elaborate?

@miellaby
Copy link
Contributor

miellaby commented Oct 2, 2017

My apologies if I missed something. But, if I understand well, when the developer puts a function value as an attribute of a "with" data-bind, this function is evaluated and its result is bound to $data? That looks odd to me. An ordinary function is not a Subscribable, so there is no reason to "unwrap" it. What if I want to pass functions as values?

@miellaby
Copy link
Contributor

miellaby commented Oct 2, 2017

Oups, my bad. Here the function is directly bound to "with", not as an attribute of "with".

So there is less a concern.

That being said, and now that I leaved lurking-mode, I can't help myself signaling that auto-unwrapping Functions and Subscribables is the aspect of KO I dislike the most.

It looks like a good idea at first, but me and every developper I trained to KO have difficulties to grasp the huge difference of "dot notations" vs "JS expressions" in bindings.

For example, the inconsistency between these 2 sequences is incomprehensible to many developpers I've dealt with:

data-bind="visible: meVisibleObservable"

data-bind="visible: !meHiddenObservable()"

So I teached all my team mates to always put parenthesis after a observable, and never relay on observable auto-unwrapping, as it leads to evolution bugs every time someone turns an auto-unwrapped dot notation into an expression.

@mbest
Copy link
Member

mbest commented Oct 2, 2017

So I teached all my team mates to always put parenthesis after a observable, and never relay on observable auto-unwrapping, as it leads to evolution bugs every time someone turns an auto-unwrapped dot notation into an expression.

This works for one-way bindings, but not for two-way bindings such as value, checked, etc. In general, it's better to pass the observable itself to a binding.

@brianmhunt
Copy link
Member

Just as a matter of interest, in tko's data-bind parser when it evaluates any javascript operation (including unary !) it auto-unwraps. So visible: !meHiddenObservable works as expected.

@miellaby
Copy link
Contributor

miellaby commented Oct 3, 2017

@brianmhunt this enhancement obviously changes the deal. But does the parser silently deals with missing attributes like in Angular, or is there still a way to handle observable reference by itself without unwrapping? because I'm accustomed now to build up expressions based on observable references. For example, I put many regular JS guards inside bindings, à la "disabled: $data.readOnly && readOnly()" (yet another case of mandatory parenthesis in ko by the way) so to implement reusable views with optional data model features.

@mbest

This works for one-way bindings, but not for two-way bindings such as value, checked, etc. In general, it's better to pass the observable itself to a binding.

I I would like to disagree, if I may. I find it more hygienic distinguishing observable references from observable unwrapped values. The difference between "text: observable()", or "value: someObservable" conveys a meaning which is useful for the general comprehension IMHO. I suppose my opinion on that is due to my low-level programming background. My apologies.

On a side node, does "value: observableReferenceObservable()" works as I expect in production KO? I won't recommend it of course, but you get the idea: according to me, such a sequence makes more sens with a consistent parenthesis unwrapping rule.

@chrisknoll
Copy link

On a side node, does "value: observableReferenceObservable()" works as I expect in production KO? I won't recommend it of course, but you get the idea: according to me, such a sequence makes more sens with a consistent parenthesis unwrapping rule.

My understanding of this case, and please others correct me if i'm wrong, when you bind to the de-refrenced observableReferenceObservable (by accessing it via ()). Your binding is to the underlying value, not an observable so changing the reference to somethign else via observableReferenceObservable(otherValue) will not signal an update to the tracker. If the inner object is an observable, i think the dependency tracking will capture change in the inner object and push a change notification if the inner observable is set. But I'm assuming your observableReferenceObservable is wrapping a raw value (not an observable).

I agree that sometimes the () notation around observables looks painful, but I've come to appreciate knowing exactly when you intended to de-ref an observable vs not. I think other frameworks treat everything as 'observable' but nice thing about having that fine-grained control over which things are actually observable makes the overall app more efficient.

@brianmhunt
Copy link
Member

@miellaby Tko auto-unwraps only when there's an operator (because operations on the observable functions are generally nonsensical). So $data.readOnly && readOnly() could be $data.readOnly && readOnly, or using the new @ operator just @readOnly. The old way still works, but the new way has semantic sugar.

@fastfasterfastest
Copy link

@miellaby

An ordinary function is not a Subscribable, so there is no reason to "unwrap" it

A little late to the party... but I agree.

I didn't see this issue when I opened #2466

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

6 participants