emberjs / ember.js

Ember.js - A JavaScript framework for creating ambitious web applications
https://emberjs.com
MIT License
22.47k stars 4.21k forks source link

Improve "Backtracking re-render" assertion #14709

Closed GavinJoyce closed 7 years ago

GavinJoyce commented 7 years ago

I've a few backtracking re-render issues to solve in my app and I'm interested in improving the assertions to make this easier for me and others. This issue will serve as a place to capture any discussion or progress.

The current assertion message for this example looks like:

Assertion Failed: You modified model.name twice on [object Object] in a single render. This was unreliable and slow in Ember 1.x and is no longer supported...

It would be nice to improve this to include more information on where the property was originally consumed as well as where it was then mutated. Something like this would be nice to aim for:

Assertion Failed. You rendered "model.name" in "templates/example1.hbs" and modified it in "components/component-that-changes-name-on-init" in a single render. This was unreliable and slow in Ember 1.x and is no longer supported...

Here are some thoughts from @chancancode on how we might do this.

https://embercommunity.slack.com/archives/dev-ember/p1481392322000438:

screen shot 2016-12-11 at 16 34 45

Useful links:

GavinJoyce commented 7 years ago

For the You rendered "model.name" in "templates/example1.hbs" part, we should aim to include this data in meta when in debug mode so that we can use it when we detect a set on an already rendered property.

Looking at the available context where we currently set lastRenderedFrom[key], we don't appear to have easy access to anything that would allow us to identify the source template:

screen shot 2016-12-11 at 17 11 33

We have easy access to:

screen shot 2016-12-11 at 17 21 23

It's not immediately clear to me how we can get any source template details from this. Some ideas the I'm going to explore:

GavinJoyce commented 7 years ago

https://embercommunity.slack.com/archives/dev-ember/p1481478414000479

screen shot 2016-12-11 at 17 54 55
GavinJoyce commented 7 years ago

We have a reference when setting lastRenderedFrom. We also have easy access to the transaction context:

screen shot 2016-12-11 at 18 20 46

Somewhere within the context, we have an opcode which also uses the same reference:

screen shot 2016-12-11 at 18 16 33

This gives us access to bounds which includes pointers into the DOM. We can then determine which view this DOM belongs to and use this for the better assertion message.

Possible next steps:

chancancode commented 7 years ago

@GavinJoyce another idea: we can potentially use CurlyComponentManager#create and CurlyComponentManager#didRenderLayout as a pair and keep a stack of "current component" (we currently use that for the component instrumentation, so I think it could work). So at least we can tell you which component's template was causing issues (i.e. that should have enough information to report "...user.foo was first read while reading component:user-profile and later modified while rendering component:another-thing).

Probably need to add more instrumentation hooks to Glimmer if we want to get more granular than that.

GavinJoyce commented 7 years ago

@chancancode thanks, I've created a quick spike (😷 using a global to track the last rendered component 😷) and it works pretty well in our app:

Before:

You modified message.message_goal.description twice on <(subclass of Ember.Model):ember3486> in a single render. This was unreliable and slow in Ember 1.x and is no longer supported. See https://github.com/emberjs/ember.js/issues/13948 for more details.

After:

Assertion Failed: You rendered message.message_goal.description in component:message-edit-expanding-container-component and modified it in component:rules/predicate-date-value-component (<(subclass of Ember.Model):ember3486>) in a single render. This was unreliable and slow in Ember 1.x and is no longer supported. See https://github.com/emberjs/ember.js/issues/13948 for more details.

Next Steps:

kiwiupover commented 7 years ago

Thanks heaps Mr Joyce. @gavinjoyce