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

[GLIMMER] Prevent auto-run assertion for objects not rendered. #14091

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 19, 2016

Prior to this change any Ember.set done after any rendering has been done during tests will trigger the auto-run assertion.

This change prevents the auto-run assertion when a given object has not been used in rendering process.

Prior to this change any `Ember.set` done after **any** rendering has
been done during tests will trigger the auto-run assertion.

This change prevents the auto-run assertion when a given object has not
been used in rendering process.
@rwjblue
Copy link
Member Author

rwjblue commented Aug 19, 2016

@wycats / @krisselden - Would you mind reviewing this? If this is not correct, can you explain what I am forgetting/missing?

@rwjblue
Copy link
Member Author

rwjblue commented Aug 19, 2016

FWIW, this fixes the errors under ember-alpha for ember-data (identified in emberjs/data#4503).

@chancancode
Copy link
Member

I am 90% sure that this is fine, in fact we perhaps don't even need to bump CURRENT_TAG anymore (since nothing in the system uses it); however, I would appreciate more eyes and brains on this

@krisselden
Copy link
Contributor

I have doubts, i'd like to know the motivation

@rwjblue
Copy link
Member Author

rwjblue commented Aug 19, 2016

There are any number of things that you may be doing that trigger a propertyDidChange event that have nothing to do with rendering.

One example is a simply creating an array proxy (new Ember.ArrayProxy({ content: []});), before this PR this would trigger the auto-run assertion if any views had been rendered (demo here). That seems quite wild (and potentially breaking) to me.

@krisselden
Copy link
Contributor

"There are any number of things that you may be doing that trigger a propertyDidChange event that have nothing to do with rendering."

This is a given, that is how it works now.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 19, 2016

I'm not sure I follow.

On current alpha builds (before the change in this PR), anything that calls propertyDidChange (regardless of if it were used in the templating system) is going to trigger the auto-run assertion in test mode.

This is definitely a change from current beta. I am unsure if it will affect folks trying to test glimmer or not...

@krisselden
Copy link
Contributor

After thinking about it, if the view layer puts the tag onto anything it renders and CURRENT_TAG is unused by the view system, and observers then are used behind the tagged object going from view system to object layer, I think this change is good, but it should return early, not dirty CURRENT_TAG and we have to make sure we have no more CURRENT_TAG usage in the view layer.

@rwjblue
Copy link
Member Author

rwjblue commented Aug 19, 2016

Replaced by #14093.

@rwjblue rwjblue closed this Aug 19, 2016
@rwjblue rwjblue deleted the prevent-auto-run-assertions-for-objects-not-in-templates branch August 19, 2016 12:55
@homu
Copy link
Contributor

homu commented Aug 19, 2016

☔ The latest upstream changes (presumably #14093) made this pull request unmergeable. Please resolve the merge conflicts.

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.

4 participants