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

[BUGFIX release] Faster attrs-proxy #11946

Merged
merged 1 commit into from
Aug 2, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -300,16 +300,15 @@ ComponentNodeManager.prototype.destroy = function() {

export function createComponent(_component, isAngleBracket, _props, renderNode, env, attrs = {}, proto = _component.proto()) {
let props = assign({}, _props);
let attrsSnapshot;

if (!isAngleBracket) {
let hasSuppliedController = 'controller' in attrs; // 2.0TODO remove
Ember.deprecate('controller= is deprecated', !hasSuppliedController, { id: 'ember-htmlbars.create-component', until: '3.0.0' });

attrsSnapshot = takeSnapshot(attrs);
props.attrs = attrsSnapshot;
let snapshot = takeSnapshot(attrs);
props.attrs = snapshot;

mergeBindings(props, shadowedAttrs(proto, attrsSnapshot));
mergeBindings(props, shadowedAttrs(proto, snapshot));
} else {
props._isAngleBracket = true;
}
Expand All @@ -319,8 +318,6 @@ export function createComponent(_component, isAngleBracket, _props, renderNode,

let component = _component.create(props);

env.renderer.componentInitAttrs(component, attrsSnapshot);

// for the fallback case
component.container = component.container || env.container;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ ViewNodeManager.prototype.render = function(env, attrs, visitor) {
}

if (component) {
var snapshot = takeSnapshot(attrs);
env.renderer.setAttrs(this.component, snapshot);
env.renderer.willRender(component);
env.renderedViews.push(component.elementId);
}
Expand Down Expand Up @@ -130,7 +128,7 @@ ViewNodeManager.prototype.rerender = function(env, attrs, visitor) {
env.renderer.willUpdate(component, snapshot);

if (component._renderNode.shouldReceiveAttrs) {
env.renderer.updateAttrs(component, snapshot);
env.renderer.componentUpdateAttrs(component, snapshot);
setProperties(component, mergeBindings({}, shadowedAttrs(component, snapshot)));
component._renderNode.shouldReceiveAttrs = false;
}
Expand Down Expand Up @@ -187,6 +185,7 @@ export function createOrUpdateComponent(component, options, createOptions, rende

component = component.create(props);
} else {
env.renderer.componentUpdateAttrs(component, snapshot);
mergeBindings(props, shadowedAttrs(component, snapshot));
setProperties(component, props);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -810,55 +810,6 @@ QUnit.test('implementing `render` allows pushing into a string buffer', function
equal(view.$('#zomg').text(), 'Whoop!');
});

QUnit.test('comopnent should rerender when a property is changed during children\'s rendering', function() {
expectDeprecation(/twice in a single render/);

var outer, middle;

registry.register('component:x-outer', Component.extend({
value: 1,
grabReference: Ember.on('init', function() {
outer = this;
})
}));

registry.register('component:x-middle', Component.extend({
grabReference: Ember.on('init', function() {
middle = this;
})
}));

registry.register('component:x-inner', Component.extend({
pushDataUp: Ember.observer('value', function() {
middle.set('value', this.get('value'));
})
}));

registry.register('template:components/x-outer', compile('{{#x-middle}}{{x-inner value=value}}{{/x-middle}}'));
registry.register('template:components/x-middle', compile('<div id="middle-value">{{value}}</div>{{yield}}'));
registry.register('template:components/x-inner', compile('<div id="inner-value">{{value}}</div>'));


view = EmberView.extend({
template: compile('{{x-outer}}'),
container: container
}).create();

runAppend(view);

equal(view.$('#inner-value').text(), '1', 'initial render of inner');
equal(view.$('#middle-value').text(), '1', 'initial render of middle');

run(() => outer.set('value', 2));

equal(view.$('#inner-value').text(), '2', 'second render of inner');
equal(view.$('#middle-value').text(), '2', 'second render of middle');

run(() => outer.set('value', 3));

equal(view.$('#inner-value').text(), '3', 'third render of inner');
equal(view.$('#middle-value').text(), '3', 'third render of middle');
});

QUnit.test('components in template of a yielding component should have the proper parentView', function() {
var outer, innerTemplate, innerLayout;
Expand Down Expand Up @@ -970,7 +921,7 @@ QUnit.test('components should receive the viewRegistry from the parent view', fu
equal(outer._viewRegistry, viewRegistry);
});

QUnit.test('comopnent should rerender when a property (with a default) is changed during children\'s rendering', function() {
QUnit.test('comopnent should rerender when a property is changed during children\'s rendering', function() {
expectDeprecation(/modified value twice in a single render/);

var outer, middle;
Expand Down
66 changes: 21 additions & 45 deletions packages/ember-views/lib/compat/attrs-proxy.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
import { get } from 'ember-metal/property_get';
import { Mixin } from 'ember-metal/mixin';
import { on } from 'ember-metal/events';
import { symbol } from 'ember-metal/utils';
import { PROPERTY_DID_CHANGE } from 'ember-metal/property_events';

import {
addObserver,
removeObserver,
_addBeforeObserver,
_removeBeforeObserver
} from 'ember-metal/observer';
import { on } from 'ember-metal/events';

export function deprecation(key) {
return `You tried to look up an attribute directly on the component. This is deprecated. Use attrs.${key} instead.`;
Expand All @@ -21,16 +13,6 @@ function isCell(val) {
return val && val[MUTABLE_CELL];
}

function attrsWillChange(view, attrsKey) {
let key = attrsKey.slice(6);
view.currentState.legacyAttrWillChange(view, key);
}

function attrsDidChange(view, attrsKey) {
let key = attrsKey.slice(6);
view.currentState.legacyAttrDidChange(view, key);
}

let AttrsProxyMixin = {
attrs: null,

Expand All @@ -56,46 +38,39 @@ let AttrsProxyMixin = {
val.update(value);
},

willWatchProperty(key) {
if (this._isAngleBracket || key === 'attrs') { return; }

let attrsKey = `attrs.${key}`;
_addBeforeObserver(this, attrsKey, null, attrsWillChange);
addObserver(this, attrsKey, null, attrsDidChange);
_propagateAttrsToThis() {
let attrs = this.attrs;
let values = {};
for (let prop in attrs) {
if (prop !== 'attrs') {
values[prop] = this.getAttr(prop);
}
}
this.setProperties(values);
Copy link
Member

Choose a reason for hiding this comment

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

although strange, set in a loop is typically faster, since the batching of setProperties comes at a cost.

Worth testing (if someone has time)

},

didUnwatchProperty(key) {
if (this._isAngleBracket || key === 'attrs') { return; }
initializeShape: on('init', function() {
this._isDispatchingAttrs = false;
}),

let attrsKey = `attrs.${key}`;
_removeBeforeObserver(this, attrsKey, null, attrsWillChange);
removeObserver(this, attrsKey, null, attrsDidChange);
didReceiveAttrs() {
this._super();
this._isDispatchingAttrs = true;
this._propagateAttrsToThis();
this._isDispatchingAttrs = false;
},

legacyDidReceiveAttrs: on('didReceiveAttrs', function() {
if (this._isAngleBracket) { return; }

var keys = Object.keys(this.attrs);

for (var i = 0, l = keys.length; i < l; i++) {
// Only issue the deprecation if it wasn't already issued when
// setting attributes initially.
if (!(keys[i] in this)) {
this.notifyPropertyChange(keys[i]);
}
}
}),

unknownProperty(key) {
if (this._isAngleBracket) { return; }

var attrs = get(this, 'attrs');
var attrs = this.attrs;
Copy link
Member

Choose a reason for hiding this comment

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

yay 20% less gets FTW


if (attrs && key in attrs) {
// do not deprecate accessing `this[key]` at this time.
// add this back when we have a proper migration path
// Ember.deprecate(deprecation(key), { id: 'ember-views.', until: '3.0.0' });
let possibleCell = get(attrs, key);
let possibleCell = attrs.key;

if (possibleCell && possibleCell[MUTABLE_CELL]) {
return possibleCell.value;
Expand All @@ -112,6 +87,7 @@ let AttrsProxyMixin = {

AttrsProxyMixin[PROPERTY_DID_CHANGE] = function(key) {
if (this._isAngleBracket) { return; }
if (this._isDispatchingAttrs) { return; }

if (this.currentState) {
this.currentState.legacyPropertyDidChange(this, key);
Expand Down
19 changes: 0 additions & 19 deletions packages/ember-views/lib/views/states/default.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
import EmberError from 'ember-metal/error';
import { get } from 'ember-metal/property_get';

import {
propertyWillChange,
propertyDidChange
} from 'ember-metal/property_events';

import { MUTABLE_CELL } from 'ember-views/compat/attrs-proxy';

/**
Expand All @@ -26,21 +20,8 @@ export default {
return null;
},

legacyAttrWillChange(view, key) {
if (key in view.attrs && !(key in view)) {
propertyWillChange(view, key);
}
},

legacyAttrDidChange(view, key) {
if (key in view.attrs && !(key in view)) {
propertyDidChange(view, key);
}
},

legacyPropertyDidChange(view, key) {
let attrs = view.attrs;

if (attrs && key in attrs) {
let possibleCell = attrs[key];

Expand Down
2 changes: 0 additions & 2 deletions packages/ember-views/lib/views/states/pre_render.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import merge from 'ember-metal/merge';
let preRender = Object.create(_default);

merge(preRender, {
legacyAttrWillChange(view, key) {},
legacyAttrDidChange(view, key) {},
legacyPropertyDidChange(view, key) {}
});

Expand Down
2 changes: 2 additions & 0 deletions packages/ember-views/lib/views/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -1322,6 +1322,8 @@ var View = CoreView.extend(
if (!this._viewRegistry) {
this._viewRegistry = View.views;
}

this.renderer.componentInitAttrs(this, this.attrs || {});
},

__defineNonEnumerable(property) {
Expand Down
1 change: 0 additions & 1 deletion packages/ember-views/tests/compat/attrs_proxy_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ QUnit.test('an observer on an attribute in the root of the component is fired wh

barObserver: on('init', observer('bar', function() {
var count = get(this, 'observerFiredCount');

set(this, 'observerFiredCount', count + 1);
})),

Expand Down