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

Fix multiplicative observation of controllers in views #12414

Merged
merged 2 commits into from
Oct 1, 2015

Conversation

trentmwillis
Copy link
Member

Addresses issue #12016.

Root cause: when the controller changed for a view, it would notify all of it's descendant views which would in turn notify all of their descendant views resulting in a multiplication of work, especially when many descendants are involved.

Solution: when the controller changes for a view, only notify it's immediate children.

@stefanpenner
Copy link
Member

Note: I haven't added a test for this yet, as I wasn't sure if I should since this is marked as legacy behavior at this point. Can add it if needed.

A test would be ideal, that way if we find a regression or need to build on it, we have some confidence in our changes.

Thank you for digging into this.

@trentmwillis
Copy link
Member Author

@stefanpenner added a test and verified it failed before this fix.

@stefanpenner
Copy link
Member

@trentmwillis very nice, thank you for taking the time.

@wycats @ef4 @tomdale @krisselden I would love to double check this solution with one of you

@ef4
Copy link
Contributor

ef4 commented Oct 1, 2015

Looks good to me.

stefanpenner added a commit that referenced this pull request Oct 1, 2015
Fix multiplicative observation of controllers in views
@stefanpenner stefanpenner merged commit 72bdb39 into emberjs:master Oct 1, 2015
@stefanpenner
Copy link
Member

Oops. These should have been bugfix release....

@rwjblue paging you to fix my mistake :p

@rwjblue
Copy link
Member

rwjblue commented Oct 1, 2015 via email

@ef4
Copy link
Contributor

ef4 commented Oct 1, 2015

I would favor release-1-13, unless there is some hurdle that makes the backport nontrivial.

@stefanpenner
Copy link
Member

I would favor release-1-13, unless there is some hurdle that makes the backport nontrivial.

@green-arrow
Copy link
Contributor

@stefanpenner or @ef4 - was this put in a 1.13 release? I was looking through the changelog and didn't see this issue referenced anywhere.

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2015

Yeah, I missed this (since it didn't have a commit prefix).

@rwjblue
Copy link
Member

rwjblue commented Nov 18, 2015

Just pushed to release-1-13 branch. Assuming that the Travis build passes properly, the builds will be available from bower via "ember": "release-1-13" in a few minutes.

@green-arrow
Copy link
Contributor

Thanks for that @rwjblue. I'll make sure to pull that down and give it a look once the build goes through.

@green-arrow
Copy link
Contributor

@rwjblue - pulled down that release and all is working well. At least the issue I was having with these transitions are resolved 😄

@green-arrow
Copy link
Contributor

@rwjblue any idea when this will be cut in a 1.13 release? Not sure if you were waiting on anything else before cutting a release.

@rwjblue
Copy link
Member

rwjblue commented Nov 20, 2015

I don't see a bunch of value in tagging for just one change, when the builds (both S3 and bower) are available. I generally batch a few changes up before releasing...

@green-arrow
Copy link
Contributor

That's fair. I'll just continue with the release-1-13 build then. Thanks again!

@pavloo
Copy link

pavloo commented Dec 2, 2015

I confirm that this fixed my problem with poor rendering performance of the component in the app I was working on.

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

Successfully merging this pull request may close these issues.

6 participants