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

Dynamically displayed component being rendered more than once #2163

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

mbest
Copy link
Member

@mbest mbest commented Dec 31, 2016

codymullins:

I've been battling this one for a few weeks, and just now got confirmation that someone else was able to reproduce this and I wasn't just going crazy.

The issue arises when I am dynamically displaying a component based on some logic - in this case, I'm parsing the route and grabbing a parameter. When the component is looking at an observable to determine the name, it's being loaded twice in the getComponent call and thus my constructor is called twice. If I bind the same form with a literal string, the component only renders once.

Here's an example (it has the same issue even without the outer if binding):

<!-- WORKS, renders only once -->
<div data-bind='if: currentFormComponent() != null'>
    <div data-bind='component: {name: "my-component", params: { route: pageParams.route }}'></div>
</div>

<!-- Renders twice (currentFormComponent is an observable and is set to "my-component" as above) -->
<div data-bind='if: currentFormComponent() != null'>
    <div data-bind='component: {name: currentFormComponent, params: { route: pageParams.route }}'></div>
</div>

@mbest
Copy link
Member

mbest commented Dec 2, 2016

Can you put together an example in jsfiddle?

@codymullins
Copy link
Author

Sure. I'll do it this afternoon after work.

@mbest mbest modified the milestone: Not assigned Dec 2, 2016
@mbest mbest added the waiting label Dec 9, 2016
@codymullins
Copy link
Author

@mbest I am trying to wrap this up this morning, but it revolves around loading the component using something like require. I'm having a difficult time getting it to work in JsFiddle. Would something you can clone from github work?

@mbest
Copy link
Member

mbest commented Dec 28, 2016

@codymullins Yeah I'll give that a shot.

@codymullins
Copy link
Author

@mbest I might have just found out where this is coming from. I turned off deferred updates and the problem is fixed. I think I read somewhere it was expected with deferred updates that a component could be rendered twice?

@mbest
Copy link
Member

mbest commented Dec 30, 2016

I don't know why it would be expected. Perhaps this is related to #2174. Could you try applying that patch and see if it makes a difference?

@codymullins
Copy link
Author

codymullins commented Dec 30, 2016

I pulled down that branch and built it, doesn't appear to make a difference. If I call ko.tasks.runEarly() right after setting the component observable though it seems the problem is also solved. It's almost like because the component loader is taking some time to load the component, it gets attempted 2x.

@mbest
Copy link
Member

mbest commented Dec 30, 2016

Thanks for checking that out and the additional information. Can you share the code that sets the observable?

@codymullins
Copy link
Author

hey @mbest I don't know why I couldn't replicate it before, but here's what I'm experiencing:

https://jsfiddle.net/11umxthr/

If you open up the console before you click the button, you'll see the console.log statement gets executed twice. if you uncomment ko.tasks.runEarly(), you'll see it does not.

@mbest
Copy link
Member

mbest commented Dec 31, 2016

Why are you setting currentFileComponent to one thing and then changing it right away?

@codymullins
Copy link
Author

codymullins commented Dec 31, 2016

In this scenario there's some logic to determine which component to display based on what's in the route. I think previously I was using an if: binding and setting the currentFileComponent to null at least until one was chosen. (the example doesn't include that, I whittled it down to bare bones).

@mbest
Copy link
Member

mbest commented Dec 31, 2016

Your example exposes an issue that when using deferred updates a subscriber can get a notification that occurred before it subscribed to the observable. When you change the observable's value, the notification is scheduled. Afterwards the component binds using the latest value. But then it gets the notification of the same value.

As you noted, a workaround is to call ko.tasks.runEarly(). That clears the notification. Another is to schedule the update for later: ko.tasks.schedule(function() { self.currentFileComponent("main-file"); }); The component binding will first use the loading component and later update to main-file.

A possible fix for this issue is for Knockout to only call subscribers that existed when the notification was scheduled.

…notify those present and not future subscriptions.
@mbest
Copy link
Member

mbest commented Dec 31, 2016

I've attached the fix here.

@mbest mbest mentioned this pull request Jan 1, 2017
@codymullins
Copy link
Author

Awesome. That was quick, you rock. I will pull this down and test it out.

@codymullins
Copy link
Author

I can confirm this fixes the issue. I don't see any other evident side effects either.

mbest added a commit that referenced this pull request Jan 2, 2017
…o only notify those present and not future subscriptions.
@mbest mbest merged commit ac94715 into master Feb 10, 2017
@mbest mbest deleted the 2163-deferred-shouldnt-notify-future-subs branch February 10, 2017 07:19
@codymullins
Copy link
Author

@mbest since this is merged in, are you planning the 3.4.2 release anytime soon?

@mbest
Copy link
Member

mbest commented Feb 13, 2017

@mbest
Copy link
Member

mbest commented Feb 13, 2017

since this is merged in, are you planning the 3.4.2 release anytime soon?

Hopefully. I just added another issue to consider for 3.4.2, plus there's a possible regression in 3.4.1 that I want to look into.

@mbest mbest modified the milestones: 3.4.2, 3.5.0 Feb 22, 2017
@mbest
Copy link
Member

mbest commented Mar 7, 2017

@codymullins 3.4.2 is now released.

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

Successfully merging this pull request may close these issues.

None yet

2 participants