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

Adds Ember.Debug.RenderDebug as an api for render debugging. #11218

Closed
wants to merge 1 commit into from

Conversation

teddyzeenny
Copy link
Contributor

Currently exposes Ember.Debug.RenderDebug.getTopLevelNode which returns the top level inspected node.
InspectedNode is a wrapper class for the htmlbars render node. It provides public debugging methods
so that external tools like the Ember Inspector can use stable and tested apis.

I'm looking for feedback on this approach (mainly the InspectedNode class). cc @rwjblue @krisselden

@teddyzeenny teddyzeenny force-pushed the render-node-debug branch 2 times, most recently from a5ffa0e to 1274406 Compare May 24, 2015 19:49
@rwjblue
Copy link
Member

rwjblue commented Jun 12, 2015

We went with Ember.Debug for the deprecation log level implementation, can you swap to using that instead of Ember.Debugging?

@teddyzeenny
Copy link
Contributor Author

Ember.Debug is probably going to conflict with the Ember Inspector's Ember.Debug. I can look into removing it from the inspector if you think it's worth it.

@mixonic
Copy link
Sponsor Member

mixonic commented Jun 12, 2015

@teddyzeenny oooomg namespaces :-/ Is is painful to rename in inspector? ember-debug-adapter?

@teddyzeenny
Copy link
Contributor Author

I should be able to rename it - maybe to EmberInspector. Ember.Debug has always been a confusing name so it may be a good thing to rename it anyway.

* @type {Object}
* @private
*/
renderNode: null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is internal and should not be leaked, it should at least have an underscore prefix and not be documented like it is a thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The downside of making this inaccessible is that I can't match render nodes that I got from perf instrumentation to the render nodes in this view tree. (I use this to add render times next to the views in the view tree). Unless if we maybe provide the render node's guid here and in perf instrumentation?

@rwjblue
Copy link
Member

rwjblue commented Aug 2, 2015

@teddyzeenny - I'd love to land this. Can you rebase, fix the issues that @krisselden mentioned?

@teddyzeenny teddyzeenny force-pushed the render-node-debug branch 2 times, most recently from 3b7bb98 to 69b8cbe Compare August 5, 2015 16:03
@teddyzeenny teddyzeenny changed the title Adds Ember.Debugging.RenderDebug as an api for render debugging. Adds Ember.Debug.RenderDebug as an api for render debugging. Aug 5, 2015
@teddyzeenny
Copy link
Contributor Author

Updated.

@rwjblue
Copy link
Member

rwjblue commented Aug 23, 2015

I am happy with this. We can always iterate as we hit problems...

@krisselden - Are you ok with this as a first iteration?

@teddyzeenny
Copy link
Contributor Author

@krisselden mentioned that the namespace Ember.Debug is something that should be removed from production builds, whereas inspection methods should not. So we would use something like Ember.Inspection instead. Downside is it adds another namespace to the Ember API. Thoughts?

@mmun
Copy link
Member

mmun commented Aug 27, 2015

@teddyzeenny I agree with that.

@rwjblue
Copy link
Member

rwjblue commented Aug 27, 2015

@teddyzeenny - Ember.Debug is not removed from production builds. The guts of our packages/ember-debug is not included, but the public API does exist (otherwise users calling functions on it would have broken apps only in prod).

@teddyzeenny
Copy link
Contributor Author

@rwjblue I think @krisselden meant there's a difference in concept between debug and inspection and therefore should have separate namespaces. @krisselden can you confirm/deny?

I'd love to get this in as soon as possible so we can prevent more regressions to the view tree as Ember changes.

@rwjblue
Copy link
Member

rwjblue commented Aug 28, 2015

@teddyzeenny - Gotcha. Yeah, I'm fine with Ember.Inspection or some such. Should we move the current debug adapter there too?

@teddyzeenny
Copy link
Contributor Author

@rwjblue yes it makes sense to move the data adapter but we should probably keep and deprecate the original Ember.DataAdapter since it was public api?

@rwjblue
Copy link
Member

rwjblue commented Aug 28, 2015

we should probably keep and deprecate the original

Yes, absolutely!

Currently exposes `Ember.Debug.RenderDebug.getTopLevelNode` which
returns the top level inspected node. `InspectedNode` is a wrapper class for
the htmlbars render node. It provides public debugging methods
so that external tools like the Ember Inspector can use stable and
tested apis.
@homu
Copy link
Contributor

homu commented Nov 29, 2015

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

@locks
Copy link
Contributor

locks commented Jan 21, 2017

hey @teddyzeenny, is this still relevant?

@teddyzeenny
Copy link
Contributor Author

Not relevant anymore. Closing.

@teddyzeenny teddyzeenny deleted the render-node-debug branch January 22, 2017 19:38
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.

7 participants