-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Improved backtracking re-render assertion message #14723
Improved backtracking re-render assertion message #14723
Conversation
b7a210e
to
1e845d5
Compare
15fe368
to
d2a0e91
Compare
@@ -107,8 +108,9 @@ export function Meta(obj, parentMeta) { | |||
|
|||
if (isEnabled('ember-glimmer-detect-backtracking-rerender') || | |||
isEnabled('ember-glimmer-allow-backtracking-rerender')) { | |||
this._lastRendered = undefined; | |||
this._lastRenderedFrom = undefined; // FIXME: not used in production, remove me from prod builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the _
prefixes as they seemed incorrect
@@ -67,7 +67,8 @@ const IS_PROXY = 1 << 4; | |||
if (isEnabled('ember-glimmer-detect-backtracking-rerender') || | |||
isEnabled('ember-glimmer-allow-backtracking-rerender')) { | |||
members.lastRendered = ownMap; | |||
members.lastRenderedFrom = ownMap; // FIXME: not used in production, remove me from prod builds | |||
members.lastRenderedReferenceMap = ownMap; // FIXME: not used in production, remove me from prod builds | |||
members.lastRenderedTemplateMap = ownMap; // FIXME: not used in production, remove me from prod builds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
runInDebug
doesn't seem to work here. I'd be grateful for any tips on how I can avoid setting these in production builds
d2a0e91
to
dd472d7
Compare
cad7f6a
to
9993a34
Compare
9993a34
to
797d92f
Compare
this is ready for review (/cc @chancancode) |
Looks great! 👍 I think there are some issues with the meta-metaprogramming usage. For example I believe the underscore was correct, and we could try to figure out the runInDebug stuff. I am currently travelling but I'm sure someone else (Robert, Kris, Edward, Stef etc) could help with those. Another thing is we probably should be pushing something in all component managers (dynamic components, render helper, mount, etc) as those are all the places we switch templates. I think we can improve the message a bit too. Maybe start with the problem summary (you modified X twice in the same render) then go into the details on the next sentence/line to make it more readable. Other than those issues, this looks great!! Thanks for working on this. Can you get some other commenters on the original thread (Stanley?) to try this out? |
updated message: You modified |
I've cut a If you'd like to try it out yourself, you can here: GavinJoyce/backtracking#10 |
8c5b6e1
to
825c67d
Compare
@GavinJoyce I love the error message. The "rendered in" and "modified in" parts will be super helpful. |
7f49d40
to
a80c995
Compare
c982807
to
83aa469
Compare
6d3c5c2
to
391bc85
Compare
This is ready for review (/cc @chancancode). Here's a collection of improved backtracking re-render errors from this build : GavinJoyce/backtracking#10 |
8922c86
to
f599dda
Compare
^ That's been addressed. This is ready for review again |
I think this is good to go, would love for a review by @wycats / @chancancode / @krisselden / @chadhietala for sanity check... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! @tomdale and I did a "pair review" session to review this, and came up with a few small tweaks/suggestions.
I think after those are taken care of this is ready to 🚢...
@@ -74,6 +74,8 @@ class MountManager { | |||
} | |||
|
|||
create(environment, { name, env }, args, dynamicScope) { | |||
runInDebug(() => this._pushToDebugStack(name, env)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be pushing with engine:${name}
instead of just name
? The other usages that I see above are all using "factory full name" semantics...
@@ -180,6 +180,8 @@ class OutletComponentManager { | |||
} | |||
|
|||
create(environment, definition, args, dynamicScope) { | |||
runInDebug(() => this._pushToDebugStack(definition, environment)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is definition
here? As mentioned above, most of the names in the debug stack are representing "factory full names", but I am not sure what this one is actually going to add.
After reading below in the _pushToDebugStack
method for OutletManager
we are grabbing definition.template.meta.moduleName
. I think we should change things around so that we are pushing like:
runInDebug(() => this._pushToDebugStack(`template:${definition.template.meta.moduleName}`, environment));
Then eventually we can make a shared base class so that each _pushToDebugStack
implementation does not have to be unique.
@@ -212,10 +216,19 @@ class OutletComponentManager { | |||
didUpdate(state) {} | |||
} | |||
|
|||
runInDebug(() => { | |||
OutletComponentManager.prototype._pushToDebugStack = function(definition, environment) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented above, but I think ideally all of the _pushToDebugStack
implementations should basically be the same (and either in this PR or in a future refactor be changed to extend from a common base class to avoid duplication).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll follow up in a subsequent PR and refactor away the duplicate _pushToDebugStack
s
@@ -203,6 +218,10 @@ class NonSingletonRenderManager extends AbstractRenderManager { | |||
controller.set('model', args.positional.at(0).value()); | |||
} | |||
|
|||
didRenderLayout() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this wasn't previously defined, you could probably define this in the AbstractRenderManager
above (and not have to define it for each of SingletonRenderManager
and NonSingletonRenderManager
).
We should also only add didRenderLayout
(for debug purposes) when running in a debug build (in production builds at the moment the current code would result in an empty function).
template: 'Hi {{person.name}} from component' | ||
}); | ||
|
||
let expectedBacktrackingMessage = /modified "model\.name" twice on \[object Object\] in a single render\. It was rendered in "routeWithError" and modified in "component:x-foo"/; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is slightly ambiguous still (e.g. "what is routeWithError
, is it the controller or the template?").
Given the suggested changes in an earlier comment, this would become template:routeWithError
which should remove the ambiguity.
117e2f3
to
d626276
Compare
d626276
to
cd11371
Compare
@rwjblue this is ready to go now I believe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you so much for working on this @GavinJoyce!
Closes #14709
This improves the backtracking re-render assertion message.
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 #13948 for more details.After:
You modified
message.message_goal.description
twice on<(subclass of Ember.Model):ember3486>
in a single render. It was rendered incomponent:message-edit-expanding-container-component
and modified incomponent:rules/predicate-date-value-component
. This was unreliable and slow in Ember 1.x and is no longer supported. See #13948 for more details.Feedback TODOs:
runInDebug
to work hererunInDebug
isn't available inmeta.js
module initialization #14732Rebasethis._lastRendered = undefined
is correct (vsthis.lastRendered = undefined
)2.10.2-with-improved-backtracking-assertion
build : 2.10.2-with-improved-backtracking-assertion-and-backtracking intercom/ember#11debug stacknot needed,CurleyComponentManager
is ultimately invoked_debugContainerKey
CI test failures (likely related tofactoryFor
changes) 🍻 @rwjblueundefined
in example 1: 2.10.2-with-improved-backtracking-assertion GavinJoyce/backtracking#10 (comment)pop
: Improved backtracking re-render assertion message #14723 (comment)