templating-resources icon indicating copy to clipboard operation
templating-resources copied to clipboard

compose and activate issue

Open tdamir opened this issue 7 years ago • 13 comments

I'm submitting a bug report

  • Library Version: 1.5.1

Please tell us about your environment:

  • Operating System: Windows 10

  • Node Version: 6.9.2

  • NPM Version: 5.4.2
  • Browser: Chrome, Firefox, probably all

  • Language: all

Current behavior: Mixing repeat.for and passing the item to the compose (ie: model.bind) is producing unwanted activate calls. You can find the sample code to reproduce the issue here: https://github.com/tdamir/aurelia-compose-issue

Expected/desired behavior: Count of activate() and bind() calls should be the same.

tdamir avatar Oct 10 '17 23:10 tdamir

@tdamir if you could put a small repro in a gist we can take a look.

jdanyow avatar Oct 11 '17 04:10 jdanyow

@jdanyow I'll have a look. I think this surfaced because I did a fix for a race condition. From the repro:

<li repeat.for="$item of items">
      <compose containerless view-model="item" model.bind="{text: $item}"></compose>
</li>

Also I think you did mention in a previous issue that this model.bind="{text: $item}" results in 2 values(changes) because how it's evaluated.

StrahilKazlachev avatar Oct 11 '17 04:10 StrahilKazlachev

I've been playing with this a little bit and compose.modelChanged is called twice but only for some items. I've updated the sample app so now on first navigation, 100 items will be generated and modelChanged will be called only for 3 items. If we regenerate items by clicking on button, then count of activate calls will be as expected. If you try to re-generate 101 items, then activate will be called 102 times. But if we do the first navigation by generating 10 elements, then count of activate calls will be 10. It's strange.

tdamir avatar Oct 11 '17 17:10 tdamir

@tdamir Your above observation is with model.bind="{text: $item.text} or model.bind="$item"? You can also check this comment.

StrahilKazlachev avatar Oct 11 '17 17:10 StrahilKazlachev

The observation is with model.bind="{text: $item.text} and it looks like model.one-time solves the problem. Thanks!

tdamir avatar Oct 11 '17 18:10 tdamir

Earlier I did play around with the sample but did not see anything strange from <compose> point of view.

StrahilKazlachev avatar Oct 11 '17 18:10 StrahilKazlachev

I agree that <compose> looks fine. It activates the view model because model changed (when used like: model.bind="{text: $item.text}). I'll be more careful from now when binding object literal.

Sorry for all the trouble. It's just that we haven't had this issue with 1.4.x version. It suddenly appeared when we upgraded to the 1.5.1. Thanks again!

tdamir avatar Oct 11 '17 21:10 tdamir

@tdamir You haven't seen it before because there was an issue where fast consecutive changes were skipped some times.

StrahilKazlachev avatar Oct 12 '17 07:10 StrahilKazlachev

@StrahilKazlachev I understand. Do you have maybe idea, in case of model.bind="{text: $item.text}, why is modelChanged called only for some items? Is that expected behavior?

tdamir avatar Oct 12 '17 07:10 tdamir

I am also facing similar issues. I am composing a view with compose like below:

<compose show.bind="conditionToShow" view-model="controls/MyVwModel" 
         model.bind="{modelProp1:value1, modelProp2:value2, andSo:on}">
</compose>

And then MyVwModel looks like:

@autoinject
export class MyVwModel extends BaseI18N {
    private activated: boolean = false;

    constructor(i18n: I18N, element: Element, ea: EventAggregator,...) {
        super(i18n, element, ea);
    }

    public activate(model: any) {
        if (!this.activated) {
           // do stuffs
            this.fetchStuffsFromServer1();
            this.fetchStuffsFromServer2();
            this.activated = true;
        }
    }
    ...........
}

The activate in MyVwModel is being called twice. That's why, I have used activated to avoid executing the code twice, which can be expensive for obvious reasons. But this is a workaround, I don't particularly like.

Thus, my question, is there a solution/cleaner workaround?

Sayan751 avatar Feb 21 '18 10:02 Sayan751

@jdanyow here's your repro: https://gist.run/?id=9889f67a8bb51e345c4c304525b62f7d :) @bigopon I've updated my dependencies in a local repro, and same behavior. Thoughts?

davismj avatar Apr 10 '19 01:04 davismj

It starts here, where model.bind is a to-view binding, which calls enqueueBindingConnect(this);

The magic happens when there are more bindings being processed than the minimumImmediate threshold. This causes connect to get queued on raf here.

This all gets kicked off in the Controller.bind method. After this raf is queued, the Compose bind method is called here. Thanks to this line, the model value is already available at this time.

In summary, typically the model binding's bind() method is called synchronously and before Compose element's bind() method, which results in a single activation. However, if there are more than 100 bindings queued, the model binding's bind() method is queued with raf and thus called asynchronously and after the Compose element's bind() method, which triggers a second activation.

For most to-view bindings, I believe that a non-change will not trigger an update. Compose doesn't seem to have this logic on the model property, which causes duplicate activates. However, changing this now would be a breaking change, and many developers may expect Compose to reactivate if they push a new change to model, even if the object pushed is identical.

davismj avatar Apr 10 '19 01:04 davismj

Bumping this up. I run into the same issue, but without making use of repeat.for or show.bind.

Cry0nicS avatar May 23 '19 12:05 Cry0nicS