Skip to content

Commit

Permalink
[BUGFIX beta] Avoid extra work for Ember.Component#layout.
Browse files Browse the repository at this point in the history
---

Prior to this change, `layout` was a CP that did a few things:

* `get(this, 'layoutName')` (and if set `this.container.lookup`)
* `get(this, 'defaultLayout')`

After this change, `layout` is a simple property that avoids that extra
work.

---

Prior to this change, `defaultLayout` would actually override a layout
that came from the container, this was incorrect (literally the opposite
of what we wanted).

This corrects that logic, and ensures that we only attempt to use the
`layout` property if the layout was not found during the container
lookup (which properly mimics the behavior in 1.0 - 1.12 where `layout`
was injected into the component if a layout was found in the container).
  • Loading branch information
rwjblue committed Sep 6, 2015
1 parent 1bed4a5 commit bceed2f
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,9 @@ ComponentNodeManager.create = function ComponentNodeManager_create(renderNode, e
// If the component specifies its layout via the `layout` property
// instead of using the template looked up in the container, get it
// now that we have the component instance.
layout = get(component, 'layout') || layout;
if (!layout) {
layout = get(component, 'layout');
}

runInDebug(() => {
if (isAngleBracket) {
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-routing-views/lib/components/link-to.js
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ linkToTemplate.meta.revision = 'Ember@VERSION_STRING_PLACEHOLDER';
@private
**/
let LinkComponent = EmberComponent.extend({
defaultLayout: linkToTemplate,
layout: linkToTemplate,

tagName: 'a',

Expand Down
25 changes: 23 additions & 2 deletions packages/ember-views/lib/components/component.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Ember from 'ember-metal/core';
import { assert } from 'ember-metal/debug';
import { assert, deprecate } from 'ember-metal/debug';

import TargetActionSupport from 'ember-runtime/mixins/target_action_support';
import View from 'ember-views/views/view';
Expand Down Expand Up @@ -132,12 +132,33 @@ var Component = View.extend(TargetActionSupport, {
}),

init() {
this._super.apply(this, arguments);
this._super(...arguments);
set(this, 'controller', this);
set(this, 'context', this);

if (!this.layout && this.layoutName && this.container) {
let layoutName = get(this, 'layoutName');

this.layout = this.templateForName(layoutName);
}

// If a `defaultLayout` was specified move it to the `layout` prop.
// `layout` is no longer a CP, so this just ensures that the `defaultLayout`
// logic is supported with a deprecation
if (this.defaultLayout && !this.layout) {
deprecate(
`Specifying \`defaultLayout\` to ${this} is deprecated. Please use \`layout\` instead.`,
false,
{ id: 'ember-views.component.defaultLayout', until: '3.0.0' }
);

this.layout = this.defaultLayout;
}
},

template: null,
layoutName: null,
layout: null,

/**
If the component is currently inserted into the DOM of a parent view, this
Expand Down
8 changes: 8 additions & 0 deletions packages/ember-views/tests/views/component_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,14 @@ QUnit.test('Specifying both templateName and layoutName to a component is NOT de
equal(get(component, 'layoutName'), 'hum-drum');
});

QUnit.test('Specifying a defaultLayout to a component is deprecated', function() {
expectDeprecation(function() {
Component.extend({
defaultLayout: 'hum-drum'
}).create();
}, /Specifying `defaultLayout` to .+ is deprecated\./);
});

QUnit.test('Specifying a templateName on a component with a layoutName specified in a superclass is NOT deprecated', function() {
expectNoDeprecation();
var Parent = Component.extend({
Expand Down
66 changes: 65 additions & 1 deletion packages/ember/tests/component_registration_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ QUnit.test('Component-like invocations are treated as bound paths if neither tem
equal(Ember.$('#wrapper').text(), 'machty hello world', 'The component is composed correctly');
});

QUnit.test('Assigning templateName to a component should setup the template as a layout', function() {
QUnit.test('Assigning layoutName to a component should setup the template as a layout', function() {
expect(1);

Ember.TEMPLATES.application = compile('<div id=\'wrapper\'>{{#my-component}}{{text}}{{/my-component}}</div>');
Expand All @@ -154,6 +154,70 @@ QUnit.test('Assigning templateName to a component should setup the template as a
equal(Ember.$('#wrapper').text(), 'inner-outer', 'The component is composed correctly');
});

QUnit.test('Assigning layoutName and layout to a component should use the `layout` value', function() {
expect(1);

Ember.TEMPLATES.application = compile('<div id=\'wrapper\'>{{#my-component}}{{text}}{{/my-component}}</div>');
Ember.TEMPLATES['foo-bar-baz'] = compile('No way!');

boot(function() {
registry.register('controller:application', Ember.Controller.extend({
'text': 'outer'
}));

registry.register('component:my-component', Ember.Component.extend({
text: 'inner',
layoutName: 'foo-bar-baz',
layout: compile('{{text}}-{{yield}}')
}));
});

equal(Ember.$('#wrapper').text(), 'inner-outer', 'The component is composed correctly');
});

QUnit.test('Assigning defaultLayout to a component should set it up as a layout if no layout was found [DEPRECATED]', function() {
expect(2);

Ember.TEMPLATES.application = compile('<div id=\'wrapper\'>{{#my-component}}{{text}}{{/my-component}}</div>');

expectDeprecation(function() {
boot(function() {
registry.register('controller:application', Ember.Controller.extend({
'text': 'outer'
}));

registry.register('component:my-component', Ember.Component.extend({
text: 'inner',
defaultLayout: compile('{{text}}-{{yield}}')
}));
});
}, /Specifying `defaultLayout` to .+ is deprecated\./);

equal(Ember.$('#wrapper').text(), 'inner-outer', 'The component is composed correctly');
});

QUnit.test('Assigning defaultLayout to a component should set it up as a layout if layout was found [DEPRECATED]', function() {
expect(2);

Ember.TEMPLATES.application = compile('<div id=\'wrapper\'>{{#my-component}}{{text}}{{/my-component}}</div>');
Ember.TEMPLATES['components/my-component'] = compile('{{text}}-{{yield}}');

expectDeprecation(function() {
boot(function() {
registry.register('controller:application', Ember.Controller.extend({
'text': 'outer'
}));

registry.register('component:my-component', Ember.Component.extend({
text: 'inner',
defaultLayout: compile('should not see this!')
}));
});
}, /Specifying `defaultLayout` to .+ is deprecated\./);

equal(Ember.$('#wrapper').text(), 'inner-outer', 'The component is composed correctly');
});

QUnit.test('Using name of component that does not exist', function () {
Ember.TEMPLATES.application = compile('<div id=\'wrapper\'>{{#no-good}} {{/no-good}}</div>');

Expand Down

0 comments on commit bceed2f

Please sign in to comment.