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

compose and activate issue #323

Open
tdamir opened this issue Oct 10, 2017 · 13 comments
Open

compose and activate issue #323

tdamir opened this issue Oct 10, 2017 · 13 comments
Labels

Comments

@tdamir
Copy link

tdamir commented Oct 10, 2017

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.

@jdanyow
Copy link
Contributor

jdanyow commented Oct 11, 2017

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

@StrahilKazlachev
Copy link
Contributor

StrahilKazlachev commented Oct 11, 2017

@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.

@tdamir
Copy link
Author

tdamir commented Oct 11, 2017

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.

@StrahilKazlachev
Copy link
Contributor

StrahilKazlachev commented Oct 11, 2017

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

@tdamir
Copy link
Author

tdamir commented Oct 11, 2017

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

@StrahilKazlachev
Copy link
Contributor

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

@tdamir
Copy link
Author

tdamir commented Oct 11, 2017

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!

@StrahilKazlachev
Copy link
Contributor

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

@tdamir
Copy link
Author

tdamir commented Oct 12, 2017

@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?

@Sayan751
Copy link
Member

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?

@davismj
Copy link
Member

davismj commented Apr 10, 2019

@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
Copy link
Member

davismj commented Apr 10, 2019

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.

@Cry0nicS
Copy link

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

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

No branches or pull requests

6 participants