Skip to content

Commit

Permalink
Enable LOG_VIEW_LOOKUPS for {{outlet}} keyword.
Browse files Browse the repository at this point in the history 
Note: that {{outlet}} templates without a view instance do not get a
default view any longer (the template is just rendered).
  • Loading branch information
@rwjblue
rwjblue committed Apr 7, 2015
1 parent 5669af5 commit 89cf0a9
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
4 changes: 2 additions & 2 deletions packages/ember-application/tests/system/logging_test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ QUnit.test("do not log when template and view are missing when flag is not true"
});
});

QUnit.skip("log which view is used with a template", function() {
QUnit.test("log which view is used with a template", function() {
if (EmberDev && EmberDev.runningProdBuild) {
ok(true, 'Logging does not occur in production builds');
return;
Expand All @@ -217,7 +217,7 @@ QUnit.skip("log which view is used with a template", function() {
run(App, 'advanceReadiness');

visit('/posts').then(function() {
equal(logs['view:application'], 1, 'expected: Should log use of default view');
equal(logs['view:application'], undefined, 'expected: Should not use a view when only a template is defined');
equal(logs['view:index'], undefined, 'expected: Should not log when index is not present.');
equal(logs['view:posts'], 1, 'expected: Rendering posts with PostsView.');
});
Expand Down
7 changes: 7 additions & 0 deletions packages/ember-htmlbars/lib/keywords/outlet.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import merge from "ember-metal/merge";
import { get } from "ember-metal/property_get";
import ComponentNode from "ember-htmlbars/system/component-node";

import topLevelViewTemplate from "ember-htmlbars/templates/top-level-view";
Expand Down Expand Up @@ -54,6 +55,8 @@ export default {
var parentView = env.view;
var outletState = state.outletState;
var toRender = outletState.render;
var namespace = env.container.lookup('application:main');
var LOG_VIEW_LOOKUPS = get(namespace, 'LOG_VIEW_LOOKUPS');

var ViewClass = outletState.render.ViewClass;

Expand All @@ -64,6 +67,10 @@ export default {
isOutlet: true
};

if (LOG_VIEW_LOOKUPS && ViewClass) {
Ember.Logger.info("Rendering " + toRender.name + " with " + ViewClass, { fullName: 'view:' + toRender.name });
}

var componentNode = ComponentNode.create(renderNode, env, {}, options, parentView, null, null, template);
state.componentNode = componentNode;

Expand Down

8 comments on commit 89cf0a9

@mitchlloyd
Copy link
Contributor

Choose a reason for hiding this comment

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

@rwjblue If outlet templates no longer get a default view, does that mean that the markup between Glimmer and current Ember will be different?

I'm finding a test assertion that appears to expect a <div class='ember-view'> tag for an outlet:

https://github.com/emberjs/ember.js/blob/idempotent-rerender/packages/ember/tests/component_registration_test.js#L72

Based on some debugging on master, the first <div class="ember-view"> in that assertion appears to be the generated ApplicationView. On the Glimmer branch this element is replaced with a tagless OutletView. Also your assertion change here appears to indicate that application view should not be rendered.

I'm not trying to suggest there's a problem here, I'm just trying to figure out if removing the first part of the selector in the component_registration_test shown above is the right way to make that test pass.

@mixonic
Copy link
Member

Choose a reason for hiding this comment

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

@mitchlloyd OutletView extends metamorph_view, which has two properties: It is virtual (not in the tree hierarchy) and tagless.

I expect that outlets have no view so that they match this behavior: Not present in the view hierarchy (no view) and tagless (no element).

If there is some case where my reading of the code is wrong and we used to have elements for outlets, then they should continue to have elements. We can do this without a view though.

@ef4
Copy link
Contributor

@ef4 ef4 commented on 89cf0a9 Apr 20, 2015

Choose a reason for hiding this comment

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

On master, outlets are always OutletViews (which are tagless) and they always build a child view. Usually the child is view:default (which is tagless). But the topmost outlet uses view:toplevel (which is not tagless) instead. That's the tag you see at the top of an application running master.

Outlets also accept the view and viewClass parameters, which let people provide a custom view. They're expected to inherit from Ember.OutletView (which is tagless -- we do not publicly expose an outlet view implementation that is not tagless).

On glimmer, most outlets don't have views at all. OutletView is still used, but only for the topmost outlet, and there it's only so that it can effectively broadcast route changes to all the other outlets by dirtying them at the appropriate time.

It does look to me like we have an incompatible change here. We should guarantee a topmost application view. The easiest way to do it would be to change ember-routing/lib/system/router.js so that when it creates the topmost outlet, it passes tagName: 'div' (incidentally, I see that it is still using a vestigial _isTopLevel parameter, which is totally ignored).

It would be tempting to solve the problem by just making OutletView have a tag, because we only use OutletView at the top level. But that would also break compatibility for anyone who is actually inheriting from OutletView.

@mitchlloyd
Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for the insights here. The fact that only the top level outlet has an OutletView helps me make sense of what I've seen. I've tried setting the OutletView tagName to 'div' and I'm running into a few issues (Mostly Ember.View.views should be empty errors from ember-dev. I'll see if I can work through the test failures.

@ef4
Copy link
Contributor

@ef4 ef4 commented on 89cf0a9 Apr 20, 2015

Choose a reason for hiding this comment

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

I am taking a look at those test issues. I'm pretty sure it's because the tests themselves are buggy. The first one I looked at it creating two Application instances and only properly cleaning up one of them.

@mitchlloyd
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah that makes sense. I noticed that views with tagName === '' aren't _registered() which is probably why these test issues didn't show up before

https://github.com/emberjs/ember.js/blob/idempotent-rerender/packages/ember-views/lib/views/states/in_dom.js#L18-L20

Sounds like you've got this well in-hand, I'll leave this one alone.

@ef4
Copy link
Contributor

@ef4 ef4 commented on 89cf0a9 Apr 20, 2015

Choose a reason for hiding this comment

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

For completeness: I've also discovered that it's not good enough to make the top level outlet itself have a tag. That would introduce an extra layer of tags in the situation where the ApplicationView already got defined to have a tag.For compatibility, we have to make sure it's not the top outlet but the top outlet's child that is always a view with a tag.

@ef4
Copy link
Contributor

@ef4 ef4 commented on 89cf0a9 Apr 20, 2015

Choose a reason for hiding this comment

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

Fixed this in 5c5585d

Please sign in to comment.