From d78c0dd1415ed6b49ab97b966ef650e8d5be3b35 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Thu, 25 Aug 2016 05:17:58 -0700 Subject: [PATCH 1/7] Use a dictionary for the view registry --- packages/ember-application/lib/system/application.js | 3 ++- packages/ember-glimmer/tests/utils/helpers.js | 3 ++- packages/ember-htmlbars/tests/utils/helpers.js | 3 ++- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/ember-application/lib/system/application.js b/packages/ember-application/lib/system/application.js index bf96add4700..adef9f32731 100644 --- a/packages/ember-application/lib/system/application.js +++ b/packages/ember-application/lib/system/application.js @@ -4,6 +4,7 @@ */ import { ENV } from 'ember-environment'; import { assert, debug } from 'ember-metal/debug'; +import dictionary from 'ember-metal/dictionary'; import libraries from 'ember-metal/libraries'; import { isTesting } from 'ember-metal/testing'; import { get } from 'ember-metal/property_get'; @@ -1037,7 +1038,7 @@ Application.reopenClass({ }); function commonSetupRegistry(registry) { - registry.register('-view-registry:main', { create() { return {}; } }); + registry.register('-view-registry:main', { create() { return dictionary(null); } }); registry.register('route:basic', Route); registry.register('event_dispatcher:main', EventDispatcher); diff --git a/packages/ember-glimmer/tests/utils/helpers.js b/packages/ember-glimmer/tests/utils/helpers.js index b2219f1a4ff..43163b60b3b 100644 --- a/packages/ember-glimmer/tests/utils/helpers.js +++ b/packages/ember-glimmer/tests/utils/helpers.js @@ -18,6 +18,7 @@ export { DOMChanges } from 'glimmer-runtime'; export { InteractiveRenderer, InertRenderer } from 'ember-glimmer/renderer'; export { default as makeBoundHelper } from 'ember-glimmer/make-bound-helper'; export { htmlSafe, SafeString } from 'ember-glimmer/utils/string'; +import dictionary from 'ember-metal/dictionary'; export function buildOwner(options) { let owner = _buildOwner(options); @@ -37,7 +38,7 @@ export function buildOwner(options) { owner.inject('component', 'renderer', 'renderer:-dom'); owner.inject('template', 'env', 'service:-glimmer-environment'); - owner.register('-view-registry:main', { create() { return {}; } }); + owner.register('-view-registry:main', { create() { return dictionary(null); } }); owner.inject('renderer', '_viewRegistry', '-view-registry:main'); owner.register('template:-root', RootTemplate); diff --git a/packages/ember-htmlbars/tests/utils/helpers.js b/packages/ember-htmlbars/tests/utils/helpers.js index f28d6abeba7..091cffbae05 100644 --- a/packages/ember-htmlbars/tests/utils/helpers.js +++ b/packages/ember-htmlbars/tests/utils/helpers.js @@ -18,6 +18,7 @@ export { default as LinkTo } from 'ember-htmlbars/components/link-to'; export { InteractiveRenderer, InertRenderer } from 'ember-htmlbars/renderer'; export { default as makeBoundHelper } from 'ember-glimmer/make-bound-helper'; export { htmlSafe, SafeString } from 'ember-htmlbars/utils/string'; +import dictionary from 'ember-metal/dictionary'; export function buildOwner(options) { let owner = _buildOwner(options); @@ -27,7 +28,7 @@ export function buildOwner(options) { owner.inject('service:-htmlbars-environment', 'dom', 'service:-dom-helper'); owner.register('service:-document', document, { instantiate: false }); - owner.register('-view-registry:main', { create() { return {}; } }); + owner.register('-view-registry:main', { create() { return dictionary(null); } }); owner.inject('renderer', '_viewRegistry', '-view-registry:main'); owner.inject('renderer', 'dom', 'service:-dom-helper'); owner.inject('component', 'renderer', 'renderer:-dom'); From 6fc0790bb4092d71da88cda570c86d770ae46bac Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 26 Aug 2016 13:05:24 -0700 Subject: [PATCH 2/7] [CLEANUP] Remove childViews cleanup logic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is dead code, we don’t actually call these methods anywhere in the codebase anymore. --- .../lib/mixins/child_views_support.js | 34 ------------------- 1 file changed, 34 deletions(-) diff --git a/packages/ember-views/lib/mixins/child_views_support.js b/packages/ember-views/lib/mixins/child_views_support.js index 48370b982eb..2b58bba6f8e 100644 --- a/packages/ember-views/lib/mixins/child_views_support.js +++ b/packages/ember-views/lib/mixins/child_views_support.js @@ -26,36 +26,6 @@ export default Mixin.create({ this.childViews.push(view); }, - destroyChild(view) { - view.destroy(); - }, - - /** - Removes the child view from the parent view. - - @method removeChild - @param {Ember.View} view - @return {Ember.View} receiver - @private - */ - removeChild(view) { - // If we're destroying, the entire subtree will be - // freed, and the DOM will be handled separately, - // so no need to mess with childViews. - if (this.isDestroying) { return; } - - // update parent node - this.unlinkChild(view); - - // remove view from childViews array. - let { childViews } = this; - - let index = childViews.indexOf(view); - if (index !== -1) { childViews.splice(index, 1); } - - return this; - }, - linkChild(instance) { if (!instance[OWNER]) { setOwner(instance, getOwner(this)); @@ -63,9 +33,5 @@ export default Mixin.create({ instance.parentView = this; instance.ownerView = this.ownerView; - }, - - unlinkChild(instance) { - instance.parentView = null; } }); From cb429a0261378de5b19a6215f8f07e8fc21e18d8 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 26 Aug 2016 13:06:57 -0700 Subject: [PATCH 3/7] [CLEANUP] Glimmer 2 does not need `ownerView` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We also don’t need to propagate `parentView` in `linkChild` anymore: since 9228bb8, `parentView` is supplied via the “create props” for both engines to ensure that it is available during `init`. --- packages/ember-glimmer/lib/views/outlet.js | 2 - .../integration/components/life-cycle-test.js | 6 ++- .../node-managers/component-node-manager.js | 1 + .../lib/mixins/child_views_support.js | 4 -- .../tests/component_registration_test.js | 44 ++++++++++--------- 5 files changed, 28 insertions(+), 29 deletions(-) diff --git a/packages/ember-glimmer/lib/views/outlet.js b/packages/ember-glimmer/lib/views/outlet.js index 2b5e960b907..14d71bf8290 100644 --- a/packages/ember-glimmer/lib/views/outlet.js +++ b/packages/ember-glimmer/lib/views/outlet.js @@ -131,8 +131,6 @@ export default class OutletView { } appendChild(instance) { - instance.parentView = this; - instance.ownerView = this; } rerender() { diff --git a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js index 6eed11b3029..9309c9eeb4b 100644 --- a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js +++ b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js @@ -69,8 +69,10 @@ class LifeCycleHooksTest extends RenderingTest { }; let assertParentView = (hookName, instance) => { - if (!instance.parentView) { - this.assert.ok(false, `parentView should be present in ${hookName}`); + this.assert.ok(instance.parentView, `parentView should be present in ${hookName}`); + + if (this.isHTMLBars) { + this.assert.ok(instance.ownerView, `ownerView should be present in ${hookName}`); } }; diff --git a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js index 01f5380196c..65690422e99 100644 --- a/packages/ember-htmlbars/lib/node-managers/component-node-manager.js +++ b/packages/ember-htmlbars/lib/node-managers/component-node-manager.js @@ -37,6 +37,7 @@ ComponentNodeManager.create = function ComponentNodeManager_create(renderNode, e let createOptions = { parentView, + ownerView: parentView.ownerView, [HAS_BLOCK]: !!templates.default }; diff --git a/packages/ember-views/lib/mixins/child_views_support.js b/packages/ember-views/lib/mixins/child_views_support.js index 2b58bba6f8e..c11cc0abe75 100644 --- a/packages/ember-views/lib/mixins/child_views_support.js +++ b/packages/ember-views/lib/mixins/child_views_support.js @@ -18,7 +18,6 @@ export default Mixin.create({ @private */ this.childViews = []; - this.ownerView = this.ownerView || this; }, appendChild(view) { @@ -30,8 +29,5 @@ export default Mixin.create({ if (!instance[OWNER]) { setOwner(instance, getOwner(this)); } - - instance.parentView = this; - instance.ownerView = this.ownerView; } }); diff --git a/packages/ember/tests/component_registration_test.js b/packages/ember/tests/component_registration_test.js index cc5a8f5b545..f256d2d90a4 100644 --- a/packages/ember/tests/component_registration_test.js +++ b/packages/ember/tests/component_registration_test.js @@ -363,27 +363,29 @@ QUnit.test('Components trigger actions in the components context when called fro jQuery('#fizzbuzz', '#wrapper').click(); }); -QUnit.test('Components receive the top-level view as their ownerView', function(assert) { - setTemplate('application', compile('{{outlet}}')); - setTemplate('index', compile('{{my-component}}')); - setTemplate('components/my-component', compile('
')); +if (!isEnabled('ember-glimmer')) { + QUnit.test('Components receive the top-level view as their ownerView', function(assert) { + setTemplate('application', compile('{{outlet}}')); + setTemplate('index', compile('{{my-component}}')); + setTemplate('components/my-component', compile('
')); - let component; + let component; - boot(() => { - appInstance.register('component:my-component', Component.extend({ - init() { - this._super(); - component = this; - } - })); - }); + boot(() => { + appInstance.register('component:my-component', Component.extend({ + init() { + this._super(); + component = this; + } + })); + }); - // Theses tests are intended to catch a regression where the owner view was - // not configured properly. Future refactors may break these tests, which - // should not be considered a breaking change to public APIs. - let ownerView = component.ownerView; - assert.ok(ownerView, 'owner view was set'); - assert.ok(ownerView instanceof OutletView, 'owner view has no parent view'); - assert.notStrictEqual(component, ownerView, 'owner view is not itself'); -}); + // Theses tests are intended to catch a regression where the owner view was + // not configured properly. Future refactors may break these tests, which + // should not be considered a breaking change to public APIs. + let ownerView = component.ownerView; + assert.ok(ownerView, 'owner view was set'); + assert.ok(ownerView instanceof OutletView, 'owner view has no parent view'); + assert.notStrictEqual(component, ownerView, 'owner view is not itself'); + }); +} From 5840266f5f5011df007224dfb4a9ef24bfc532d7 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 26 Aug 2016 13:08:58 -0700 Subject: [PATCH 4/7] [CLEANUP] Make `ApplicationTest` have `runTask` --- .../tests/integration/components/link-to-test.js | 9 --------- packages/ember-glimmer/tests/utils/abstract-test-case.js | 8 ++++---- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/packages/ember-glimmer/tests/integration/components/link-to-test.js b/packages/ember-glimmer/tests/integration/components/link-to-test.js index 38feab36a98..ae8cc01cede 100644 --- a/packages/ember-glimmer/tests/integration/components/link-to-test.js +++ b/packages/ember-glimmer/tests/integration/components/link-to-test.js @@ -2,16 +2,11 @@ import { moduleFor, ApplicationTest } from '../../utils/test-case'; import Controller from 'ember-runtime/controllers/controller'; import Route from 'ember-routing/system/route'; import { set } from 'ember-metal/property_set'; -import run from 'ember-metal/run_loop'; import { LinkTo } from '../../utils/helpers'; import { classes as classMatcher } from '../../utils/test-helpers'; import isEnabled from 'ember-metal/features'; moduleFor('Link-to component', class extends ApplicationTest { - runTask(fn) { - run(fn); - } - visitWithDeprecation(path, deprecation) { let p; @@ -172,10 +167,6 @@ moduleFor('Link-to component with query-params', class extends ApplicationTest { } } - runTask(fn) { - run(fn); - } - ['@test populates href with fully supplied query param values'](assert) { this.registerTemplate('index', `{{#link-to 'index' (query-params foo='456' bar='NAW')}}Index{{/link-to}}`); diff --git a/packages/ember-glimmer/tests/utils/abstract-test-case.js b/packages/ember-glimmer/tests/utils/abstract-test-case.js index 88c4533c140..a4da7d1c881 100644 --- a/packages/ember-glimmer/tests/utils/abstract-test-case.js +++ b/packages/ember-glimmer/tests/utils/abstract-test-case.js @@ -109,6 +109,10 @@ export class TestCase { teardown() {} + runTask(callback) { + run(callback); + } + // The following methods require `this.element` to work get firstChild() { @@ -375,10 +379,6 @@ export class AbstractRenderingTest extends TestCase { this.component.rerender(); } - runTask(callback) { - run(callback); - } - registerHelper(name, funcOrClassBody) { let type = typeof funcOrClassBody; From 48e52b3d267d62e1c28d192a4679f25d669eb7d7 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Fri, 26 Aug 2016 22:22:28 -0700 Subject: [PATCH 5/7] Introduce a way to make ES5 getters from `Mixin`s --- packages/ember-metal/lib/descriptor.js | 27 ++ packages/ember-metal/tests/descriptor_test.js | 352 ++++++++++++++++++ 2 files changed, 379 insertions(+) create mode 100644 packages/ember-metal/lib/descriptor.js create mode 100644 packages/ember-metal/tests/descriptor_test.js diff --git a/packages/ember-metal/lib/descriptor.js b/packages/ember-metal/lib/descriptor.js new file mode 100644 index 00000000000..b51c652297e --- /dev/null +++ b/packages/ember-metal/lib/descriptor.js @@ -0,0 +1,27 @@ +import { Descriptor as EmberDescriptor } from 'ember-metal/properties'; + +export default function descriptor(desc) { + return new Descriptor(desc); +} + +/** + A wrapper for a native ES5 descriptor. In an ideal world, we wouldn't need + this at all, however, the way we currently flatten/merge our mixins require + a special value to denote a descriptor. + + @class Descriptor + @private +*/ +class Descriptor extends EmberDescriptor { + constructor(desc) { + super(); + this.desc = desc; + } + + setup(obj, key) { + Object.defineProperty(obj, key, this.desc); + } + + teardown(obj, key) { + } +} diff --git a/packages/ember-metal/tests/descriptor_test.js b/packages/ember-metal/tests/descriptor_test.js new file mode 100644 index 00000000000..b026f72e2b0 --- /dev/null +++ b/packages/ember-metal/tests/descriptor_test.js @@ -0,0 +1,352 @@ +import EmberObject from 'ember-runtime/system/object'; +import { Mixin } from 'ember-metal/mixin'; +import { defineProperty } from 'ember-metal/properties'; +import descriptor from 'ember-metal/descriptor'; + +class DescriptorTest { + + /* abstract static module(title: string); */ + + static test(title, callback) { + QUnit.test(title, assert => { + callback(assert, new this(assert)); + }); + } + + constructor(assert) { + this.assert = assert; + } + + /* abstract install(key: string, desc: Descriptor); */ + + /* abstract set(key: string, value: any); */ + + /* abstract finalize(): Object; */ +} + +let classes = [ + + class extends DescriptorTest { + static module(title) { + QUnit.module(`${title}: using defineProperty on an object directly`); + } + + constructor(assert) { + super(assert); + this.object = {}; + } + + install(key, desc) { + let { object, assert } = this; + + defineProperty(object, key, desc); + + assert.ok(object.hasOwnProperty(key)); + } + + set(key, value) { + this.object[key] = value; + } + + finalize() { + return this.object; + } + + source() { + return this.object; + } + }, + + class extends DescriptorTest { + static module(title) { + QUnit.module(`${title}: using defineProperty on a prototype`); + } + + constructor(assert) { + super(assert); + this.proto = {}; + } + + install(key, desc) { + let { proto, assert } = this; + + defineProperty(proto, key, desc); + + assert.ok(proto.hasOwnProperty(key)); + } + + set(key, value) { + this.proto[key] = value; + } + + finalize() { + return Object.create(this.proto); + } + + source() { + return this.proto; + } + }, + + class extends DescriptorTest { + static module(title) { + QUnit.module(`${title}: in EmberObject.extend()`); + } + + constructor(assert) { + super(assert); + this.klass = null; + this.props = {}; + } + + install(key, desc) { + this.props[key] = desc; + } + + set(key, value) { + this.props[key] = value; + } + + finalize() { + this.klass = EmberObject.extend(this.props); + return this.klass.create(); + } + + source() { + return this.klass.prototype; + } + }, + + class extends DescriptorTest { + static module(title) { + QUnit.module(`${title}: in EmberObject.extend() through a mixin`); + } + + constructor(assert) { + super(assert); + this.klass = null; + this.props = {}; + } + + install(key, desc) { + this.props[key] = desc; + } + + set(key, value) { + this.props[key] = value; + } + + finalize() { + this.klass = EmberObject.extend(Mixin.create(this.props)); + return this.klass.create(); + } + + source() { + return this.klass.prototype; + } + }, + + class extends DescriptorTest { + static module(title) { + QUnit.module(`${title}: inherited from another EmberObject super class`); + } + + constructor(assert) { + super(assert); + this.superklass = null; + this.props = {}; + } + + install(key, desc) { + this.props[key] = desc; + } + + set(key, value) { + this.props[key] = value; + } + + finalize() { + this.superklass = EmberObject.extend(this.props); + return this.superklass.extend().create(); + } + + source() { + return this.superklass.prototype; + } + } + +]; + +classes.forEach(TestClass => { + TestClass.module('ember-metal/descriptor'); + + TestClass.test('defining a configurable property', function(assert, factory) { + factory.install('foo', descriptor({ configurable: true, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + delete source.foo; + + assert.strictEqual(obj.foo, undefined); + + Object.defineProperty(source, 'foo', { configurable: true, value: 'baz' }); + + assert.equal(obj.foo, 'baz'); + }); + + TestClass.test('defining a non-configurable property', function(assert, factory) { + factory.install('foo', descriptor({ configurable: false, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + assert.throws(() => delete source.foo, TypeError); + + assert.throws(() => Object.defineProperty(source, 'foo', { configurable: true, value: 'baz' }), TypeError); + }); + + TestClass.test('defining an enumerable property', function(assert, factory) { + factory.install('foo', descriptor({ enumerable: true, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + assert.ok(Object.keys(source).indexOf('foo') !== -1); + }); + + TestClass.test('defining a non-enumerable property', function(assert, factory) { + factory.install('foo', descriptor({ enumerable: false, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + assert.ok(Object.keys(source).indexOf('foo') === -1); + }); + + TestClass.test('defining a writable property', function(assert, factory) { + factory.install('foo', descriptor({ writable: true, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + source.foo = 'baz'; + + assert.equal(obj.foo, 'baz'); + + obj.foo = 'bat'; + + assert.equal(obj.foo, 'bat'); + }); + + TestClass.test('defining a non-writable property', function(assert, factory) { + factory.install('foo', descriptor({ writable: false, value: 'bar' })); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + let source = factory.source(); + + assert.throws(() => source.foo = 'baz', TypeError); + + assert.throws(() => obj.foo = 'baz', TypeError); + }); + + TestClass.test('defining a getter', function(assert, factory) { + factory.install('foo', descriptor({ + get: function() { + return this.__foo__; + } + })); + + factory.set('__foo__', 'bar'); + + let obj = factory.finalize(); + + assert.equal(obj.foo, 'bar'); + + obj.__foo__ = 'baz'; + + assert.equal(obj.foo, 'baz'); + }); + + TestClass.test('defining a setter', function(assert, factory) { + factory.install('foo', descriptor({ + set: function(value) { + this.__foo__ = value; + } + })); + + factory.set('__foo__', 'bar'); + + let obj = factory.finalize(); + + assert.equal(obj.__foo__, 'bar'); + + obj.foo = 'baz'; + + assert.equal(obj.__foo__, 'baz'); + }); + + TestClass.test('combining multiple setter and getters', function(assert, factory) { + factory.install('foo', descriptor({ + get: function() { + return this.__foo__; + }, + + set: function(value) { + this.__foo__ = value; + } + })); + + factory.set('__foo__', 'foo'); + + factory.install('bar', descriptor({ + get: function() { + return this.__bar__; + }, + + set: function(value) { + this.__bar__ = value; + } + })); + + factory.set('__bar__', 'bar'); + + factory.install('fooBar', descriptor({ + get: function() { + return this.foo + '-' + this.bar; + } + })); + + let obj = factory.finalize(); + + assert.equal(obj.fooBar, 'foo-bar'); + + obj.foo = 'FOO'; + + assert.equal(obj.fooBar, 'FOO-bar'); + + obj.__bar__ = 'BAR'; + + assert.equal(obj.fooBar, 'FOO-BAR'); + + assert.throws(() => obj.fooBar = 'foobar', TypeError); + }); +}); From 4e5ddaec612997aba719b6c7e62627eb87bc96e9 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Sat, 27 Aug 2016 01:41:36 -0700 Subject: [PATCH 6/7] =?UTF-8?q?=F0=9F=98=AA=20Make=20`view.childViews`=20l?= =?UTF-8?q?azy?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🆕 `getRootViews` return an array of “top-level” components 🆕 `getChildViews` to replace `view.childViews` (to be deprecated 🔜) 🆕 “tagless” components are registered in the view-registry with a GUID 🆒 `view.childViews` is now a getter that delegates to `getChildViews` 🆓 `view.childViews` no longer leak destroyed components (bug in beta) 🔙 `view.childViews` now tracks updates (components added or removed during updates) correctly, which was unreliable since 1.13 Instead of tracking `childView` instances, we now track a list of IDs that can be used to look them up in the view registry. This has the benefit of not holding a reference to the view instance – thus do not require cleanup when the child views are destroyed. When requested, we lazily query the view registry to reify the (still) live children and cleanup and stale IDs while we are at it. This approach does leak the string IDs of destroyed components, however the effect is believed to be minimal. If this ended up causing problems, we can be smarter about this, such as tracking the number of removals and schedule a collection (outside of the render loop) when a view has accumulated enough removals. --- packages/ember-glimmer/lib/renderer.js | 12 +- .../lib/syntax/curly-component.js | 4 +- packages/ember-glimmer/lib/views/outlet.js | 3 - .../integration/components/life-cycle-test.js | 8 +- .../integration/components/utils-test.js | 234 +++++++++++++++++- packages/ember-htmlbars/lib/renderer.js | 8 +- .../lib/mixins/child_views_support.js | 33 ++- packages/ember-views/lib/system/utils.js | 74 ++++++ .../ember-views/lib/views/states/in_dom.js | 8 +- 9 files changed, 349 insertions(+), 35 deletions(-) diff --git a/packages/ember-glimmer/lib/renderer.js b/packages/ember-glimmer/lib/renderer.js index 958c146a381..cf7fc15a7bb 100644 --- a/packages/ember-glimmer/lib/renderer.js +++ b/packages/ember-glimmer/lib/renderer.js @@ -8,6 +8,7 @@ import _runInTransaction from 'ember-metal/transaction'; import isEnabled from 'ember-metal/features'; import { BOUNDS } from './component'; import { RootComponentDefinition } from './syntax/curly-component'; +import { getViewId } from 'ember-views/system/utils'; let runInTransaction; @@ -101,14 +102,14 @@ class Renderer { let self = new RootReference(view); let targetObject = view.outletState.render.controller; let ref = view.toReference(); - let dynamicScope = new DynamicScope(view, ref, ref, true, targetObject); + let dynamicScope = new DynamicScope(null, ref, ref, true, targetObject); this._renderRoot(view, view.template, self, target, dynamicScope); } appendTo(view, target) { let rootDef = new RootComponentDefinition(view); let self = new RootReference(rootDef); - let dynamicScope = new DynamicScope(view, UNDEFINED_REFERENCE, UNDEFINED_REFERENCE, true, null); + let dynamicScope = new DynamicScope(null, UNDEFINED_REFERENCE, UNDEFINED_REFERENCE, true, null); this._renderRoot(view, this._rootTemplate, self, target, dynamicScope); } @@ -126,12 +127,13 @@ class Renderer { } register(view) { - assert('Attempted to register a view with an id already in use: ' + view.elementId, !this._viewRegistry[view.elementId]); - this._viewRegistry[view.elementId] = view; + let id = getViewId(view); + assert('Attempted to register a view with an id already in use: ' + id, !this._viewRegistry[id]); + this._viewRegistry[id] = view; } unregister(view) { - delete this._viewRegistry[view.elementId]; + delete this._viewRegistry[getViewId(view)]; } remove(view) { diff --git a/packages/ember-glimmer/lib/syntax/curly-component.js b/packages/ember-glimmer/lib/syntax/curly-component.js index 13d50acf2ba..e1d145fd2c6 100644 --- a/packages/ember-glimmer/lib/syntax/curly-component.js +++ b/packages/ember-glimmer/lib/syntax/curly-component.js @@ -200,7 +200,9 @@ class CurlyComponentManager { dynamicScope.view = component; dynamicScope.targetObject = component; - parentView.appendChild(component); + if (parentView !== null) { + parentView.appendChild(component); + } component.trigger('didInitAttrs', { attrs }); component.trigger('didReceiveAttrs', { newAttrs: attrs }); diff --git a/packages/ember-glimmer/lib/views/outlet.js b/packages/ember-glimmer/lib/views/outlet.js index 14d71bf8290..86b6cf2a420 100644 --- a/packages/ember-glimmer/lib/views/outlet.js +++ b/packages/ember-glimmer/lib/views/outlet.js @@ -130,9 +130,6 @@ export default class OutletView { this._renderResult = this.renderer.appendOutletView(this, target); } - appendChild(instance) { - } - rerender() { if (this._renderResult) { this.renderer.rerender(this); } } diff --git a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js index 9309c9eeb4b..b9972760b80 100644 --- a/packages/ember-glimmer/tests/integration/components/life-cycle-test.js +++ b/packages/ember-glimmer/tests/integration/components/life-cycle-test.js @@ -2,6 +2,7 @@ import { set } from 'ember-metal/property_set'; import { Component } from '../../utils/helpers'; import { strip } from '../../utils/abstract-test-case'; import { moduleFor, RenderingTest } from '../../utils/test-case'; +import { getViewId } from 'ember-views/system/utils'; import run from 'ember-metal/run_loop'; class LifeCycleHooksTest extends RenderingTest { @@ -45,8 +46,9 @@ class LifeCycleHooksTest extends RenderingTest { assertRegisteredViews(label) { let viewRegistry = this.owner.lookup('-view-registry:main'); - let actual = Object.keys(viewRegistry).sort(); - let expected = this.componentRegistry.slice().sort(); + let topLevelId = getViewId(this.component); + let actual = Object.keys(viewRegistry).sort().filter(id => id !== topLevelId); + let expected = this.componentRegistry.sort(); this.assert.deepEqual(actual, expected, 'registered views - ' + label); } @@ -54,7 +56,7 @@ class LifeCycleHooksTest extends RenderingTest { registerComponent(name, { template = null }) { let pushComponent = (instance) => { this.components[name] = instance; - this.componentRegistry.push(instance.elementId); + this.componentRegistry.push(getViewId(instance)); }; let removeComponent = (instance) => { diff --git a/packages/ember-glimmer/tests/integration/components/utils-test.js b/packages/ember-glimmer/tests/integration/components/utils-test.js index 1fc1b6232d6..b1d90047635 100644 --- a/packages/ember-glimmer/tests/integration/components/utils-test.js +++ b/packages/ember-glimmer/tests/integration/components/utils-test.js @@ -1,11 +1,241 @@ -import { moduleFor, RenderingTest } from '../../utils/test-case'; +import Controller from 'ember-runtime/controllers/controller'; +import $ from 'ember-views/system/jquery'; +import { moduleFor, ApplicationTest, RenderingTest } from '../../utils/test-case'; import { Component } from '../../utils/helpers'; import { + getRootViews, + getChildViews, getViewBounds, getViewClientRects, getViewBoundingClientRect } from 'ember-views/system/utils'; +moduleFor('View tree tests', class extends ApplicationTest { + + constructor() { + super(); + + this.registerComponent('x-tagless', { + ComponentClass: Component.extend({ + tagName: '' + }), + + template: '
[{{id}}] {{#if isShowing}}{{yield}}{{/if}}
' + }); + + this.registerComponent('x-toggle', { + ComponentClass: Component.extend({ + isExpanded: true, + + click() { + this.toggleProperty('isExpanded'); + return false; + } + }), + + template: '[{{id}}] {{#if isExpanded}}{{yield}}{{/if}}' + }); + + let ToggleController = Controller.extend({ + isExpanded: true, + + actions: { + toggle: function() { + this.toggleProperty('isExpanded'); + } + } + }); + + this.registerController('application', ToggleController); + + this.registerTemplate('application', ` + {{x-tagless id="root-1"}} + + {{#x-toggle id="root-2"}} + {{x-toggle id="inner-1"}} + + {{#x-toggle id="inner-2"}} + {{x-toggle id="inner-3"}} + {{/x-toggle}} + {{/x-toggle}} + + + + {{#if isExpanded}} + {{x-toggle id="root-3"}} + {{/if}} + + {{outlet}} + `); + + this.registerController('index', ToggleController.extend({ + isExpanded: false + })); + + this.registerTemplate('index', ` + {{x-tagless id="root-4"}} + + {{#x-toggle id="root-5" isExpanded=false}} + {{x-toggle id="inner-4"}} + + {{#x-toggle id="inner-5"}} + {{x-toggle id="inner-6"}} + {{/x-toggle}} + {{/x-toggle}} + + + + {{#if isExpanded}} + {{x-toggle id="root-6"}} + {{/if}} + `); + + this.registerTemplate('zomg', ` + {{x-tagless id="root-7"}} + + {{#x-toggle id="root-8"}} + {{x-toggle id="inner-7"}} + + {{#x-toggle id="inner-8"}} + {{x-toggle id="inner-9"}} + {{/x-toggle}} + {{/x-toggle}} + + {{#x-toggle id="root-9"}} + {{outlet}} + {{/x-toggle}} + `); + + this.registerTemplate('zomg.lol', ` + {{x-toggle id="inner-10"}} + `); + + this.router.map(function() { + this.route('zomg', function() { + this.route('lol'); + }); + }); + } + + ['@test getRootViews'](assert) { + return this.visit('/').then(() => { + this.assertRootViews(['root-1', 'root-2', 'root-3', 'root-4', 'root-5']); + + this.runTask(() => $('#toggle-application').click()); + + this.assertRootViews(['root-1', 'root-2', 'root-4', 'root-5']); + + this.runTask(() => { + $('#toggle-application').click(); + $('#toggle-index').click(); + }); + + this.assertRootViews(['root-1', 'root-2', 'root-3', 'root-4', 'root-5', 'root-6']); + + return this.visit('/zomg/lol'); + }).then(() => { + this.assertRootViews(['root-1', 'root-2', 'root-3', 'root-7', 'root-8', 'root-9']); + + return this.visit('/'); + }).then(() => { + this.assertRootViews(['root-1', 'root-2', 'root-3', 'root-4', 'root-5', 'root-6']); + }); + } + + assertRootViews(ids) { + let owner = this.applicationInstance; + let actual = getRootViews(owner).map(view => view.id).sort(); + + if (this.isGlimmer) { + let expected = ids.sort(); + this.assert.deepEqual(actual, expected, 'root views'); + } else { + let rootView = owner.lookup('router:main')._toplevelView; + this.assert.deepEqual(actual, [rootView.id], 'root views'); + } + } + + ['@test getChildViews'](assert) { + return this.visit('/').then(() => { + this.assertChildViews('root-2', ['inner-1', 'inner-2']); + this.assertChildViews('root-5', []); + this.assertChildViews('inner-2', ['inner-3']); + + this.runTask(() => $('#root-2').click()); + + this.assertChildViews('root-2', []); + + this.runTask(() => $('#root-5').click()); + + this.assertChildViews('root-5', ['inner-4', 'inner-5']); + this.assertChildViews('inner-5', ['inner-6']); + + return this.visit('/zomg'); + }).then(() => { + this.assertChildViews('root-2', []); + this.assertChildViews('root-8', ['inner-7', 'inner-8']); + this.assertChildViews('inner-8', ['inner-9']); + this.assertChildViews('root-9', []); + + this.runTask(() => $('#root-8').click()); + + this.assertChildViews('root-8', []); + + return this.visit('/zomg/lol'); + }).then(() => { + this.assertChildViews('root-2', []); + this.assertChildViews('root-8', []); + this.assertChildViews('root-9', ['inner-10']); + + return this.visit('/'); + }).then(() => { + this.assertChildViews('root-2', []); + this.assertChildViews('root-5', []); + + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#inner-2').click()); + + this.assertChildViews('root-2', ['inner-1', 'inner-2']); + this.assertChildViews('inner-2', []); + }); + } + + ['@test getChildViews does not return duplicates'](assert) { + return this.visit('/').then(() => { + this.assertChildViews('root-2', ['inner-1', 'inner-2']); + + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + this.runTask(() => $('#root-2').click()); + + this.assertChildViews('root-2', ['inner-1', 'inner-2']); + }); + } + + assertChildViews(parentId, childIds) { + let parentView = this.viewFor(parentId); + let childViews = getChildViews(parentView); + + let actual = childViews.map(view => view.id).sort(); + let expected = childIds.sort(); + + this.assert.deepEqual(actual, expected, `child views for #${parentId}`); + } + + viewFor(id) { + let owner = this.applicationInstance; + let registry = owner.lookup('-view-registry:main'); + return registry[id]; + } +}); + let hasGetClientRects, hasGetBoundingClientRect; let ClientRectListCtor, ClientRectCtor; @@ -27,7 +257,7 @@ let ClientRectListCtor, ClientRectCtor; } })(); -moduleFor('ember-views/system/utils', class extends RenderingTest { +moduleFor('Bounds tests', class extends RenderingTest { ['@test getViewBounds on a regular component'](assert) { let component; this.registerComponent('hi-mom', { diff --git a/packages/ember-htmlbars/lib/renderer.js b/packages/ember-htmlbars/lib/renderer.js index 33526e55757..6f654c0f7e0 100755 --- a/packages/ember-htmlbars/lib/renderer.js +++ b/packages/ember-htmlbars/lib/renderer.js @@ -8,6 +8,7 @@ import { environment } from 'ember-environment'; import { internal } from 'htmlbars-runtime'; import { renderHTMLBarsBlock } from 'ember-htmlbars/system/render-view'; import fallbackViewRegistry from 'ember-views/compat/fallback-view-registry'; +import { getViewId } from 'ember-views/system/utils'; import { assert } from 'ember-metal/debug'; import { getOwner } from 'container/owner'; @@ -289,12 +290,13 @@ Renderer.prototype.getBounds = function (view) { }; Renderer.prototype.register = function Renderer_register(view) { - assert('Attempted to register a view with an id already in use: ' + view.elementId, !this._viewRegistry[view.elementId]); - this._viewRegistry[view.elementId] = view; + let id = getViewId(view); + assert('Attempted to register a view with an id already in use: ' + id, !this._viewRegistry[id]); + this._viewRegistry[id] = view; }; Renderer.prototype.unregister = function Renderer_unregister(view) { - delete this._viewRegistry[view.elementId]; + delete this._viewRegistry[getViewId(view)]; }; export const InertRenderer = { diff --git a/packages/ember-views/lib/mixins/child_views_support.js b/packages/ember-views/lib/mixins/child_views_support.js index c11cc0abe75..1b0b713291f 100644 --- a/packages/ember-views/lib/mixins/child_views_support.js +++ b/packages/ember-views/lib/mixins/child_views_support.js @@ -3,30 +3,39 @@ @submodule ember-views */ import { Mixin } from 'ember-metal/mixin'; -import { getOwner, setOwner, OWNER } from 'container/owner'; +import { getOwner, setOwner } from 'container/owner'; +import descriptor from 'ember-metal/descriptor'; +import { initChildViews, getChildViews, addChildView } from '../system/utils'; export default Mixin.create({ init() { this._super(...arguments); + initChildViews(this); + }, - /** - Array of child views. You should never edit this array directly. + /** + Array of child views. You should never edit this array directly. - @property childViews - @type Array - @default [] - @private - */ - this.childViews = []; - }, + @property childViews + @type Array + @default [] + @private + */ + childViews: descriptor({ + configurable: false, + enumerable: false, + get() { + return getChildViews(this); + } + }), appendChild(view) { this.linkChild(view); - this.childViews.push(view); + addChildView(this, view); }, linkChild(instance) { - if (!instance[OWNER]) { + if (!getOwner(instance)) { setOwner(instance, getOwner(this)); } } diff --git a/packages/ember-views/lib/system/utils.js b/packages/ember-views/lib/system/utils.js index 118578ed686..50d12f88498 100644 --- a/packages/ember-views/lib/system/utils.js +++ b/packages/ember-views/lib/system/utils.js @@ -1,5 +1,9 @@ /* globals Element */ +import { guidFor } from 'ember-metal/utils'; +import { getOwner } from 'container/owner'; +import symbol from 'ember-metal/symbol'; + /** @module ember @submodule ember-views @@ -18,6 +22,76 @@ export const STYLE_WARNING = '' + 'including how to disable this warning, see ' + 'http://emberjs.com/deprecations/v1.x/#toc_binding-style-attributes.'; +/** + @private + @method getChildViews + @param {Ember.View} view +*/ +export function getRootViews(owner) { + let registry = owner.lookup('-view-registry:main'); + + let rootViews = []; + + Object.keys(registry).forEach(id => { + let view = registry[id]; + + if (view.parentView === null) { + rootViews.push(view); + } + }); + + return rootViews; +} + +/** + @private + @method getViewId + @param {Ember.View} view + */ +export function getViewId(view) { + return view.elementId || guidFor(view); +} + +export const CHILD_VIEW_IDS = symbol('CHILD_VIEW_IDS'); +export const CHILD_VIEW_COUNTER = symbol('CHILD_VIEW_COUNTER'); + +/** + @private + @method getChildViews + @param {Ember.View} view +*/ +export function getChildViews(view) { + let owner = getOwner(view); + let registry = owner.lookup('-view-registry:main'); + return collectChildViews(view, registry); +} + +export function initChildViews(view) { + view[CHILD_VIEW_IDS] = []; +} + +export function addChildView(parent, child) { + parent[CHILD_VIEW_IDS].push(getViewId(child)); +} + +export function collectChildViews(view, registry) { + let ids = []; + let views = []; + + view[CHILD_VIEW_IDS].forEach(id => { + let view = registry[id]; + + if (view && !view.isDestroying && !view.isDestroyed && ids.indexOf(id) === -1) { + ids.push(id); + views.push(view); + } + }); + + view[CHILD_VIEW_IDS] = ids; + + return views; +} + /** @private @method getViewBounds diff --git a/packages/ember-views/lib/views/states/in_dom.js b/packages/ember-views/lib/views/states/in_dom.js index e2302d121f4..ec5f96f1f2d 100644 --- a/packages/ember-views/lib/views/states/in_dom.js +++ b/packages/ember-views/lib/views/states/in_dom.js @@ -15,9 +15,7 @@ assign(inDOM, { enter(view) { // Register the view for event handling. This hash is used by // Ember.EventDispatcher to dispatch incoming events. - if (view.tagName !== '') { - view.renderer.register(view); - } + view.renderer.register(view); runInDebug(() => { _addBeforeObserver(view, 'elementId', () => { @@ -27,9 +25,7 @@ assign(inDOM, { }, exit(view) { - if (view.tagName !== '') { - view.renderer.unregister(view); - } + view.renderer.unregister(view); } }); From 8f3c5b55938e94feb4ecc743ffaa976a9d93ea78 Mon Sep 17 00:00:00 2001 From: Godfrey Chan Date: Sat, 27 Aug 2016 23:01:41 -0700 Subject: [PATCH 7/7] =?UTF-8?q?IE9=20doesn=E2=80=99t=20seem=20to=20be=20fu?= =?UTF-8?q?lly=20spec-compliant=20here?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/ember-metal/tests/descriptor_test.js | 56 +++++++++++++++++-- 1 file changed, 52 insertions(+), 4 deletions(-) diff --git a/packages/ember-metal/tests/descriptor_test.js b/packages/ember-metal/tests/descriptor_test.js index b026f72e2b0..66f66e4522d 100644 --- a/packages/ember-metal/tests/descriptor_test.js +++ b/packages/ember-metal/tests/descriptor_test.js @@ -3,6 +3,36 @@ import { Mixin } from 'ember-metal/mixin'; import { defineProperty } from 'ember-metal/properties'; import descriptor from 'ember-metal/descriptor'; +// IE9 soft-fails when trying to delete a non-configurable property +const hasCompliantDelete = (function() { + let obj = {}; + + Object.defineProperty(obj, 'zomg', { configurable: false, value: 'zomg' }); + + try { + delete obj.zomg; + } catch(e) { + return true; + } + + return false; +})(); + +// IE9 soft-fails when trying to assign to a non-writable property +const hasCompliantAssign = (function() { + let obj = {}; + + Object.defineProperty(obj, 'zomg', { writable: false, value: 'zomg' }); + + try { + obj.zomg = 'lol'; + } catch(e) { + return true; + } + + return false; +})(); + class DescriptorTest { /* abstract static module(title: string); */ @@ -207,9 +237,15 @@ classes.forEach(TestClass => { let source = factory.source(); - assert.throws(() => delete source.foo, TypeError); + if (hasCompliantDelete) { + assert.throws(() => delete source.foo, TypeError); + } else { + delete source.foo; + } assert.throws(() => Object.defineProperty(source, 'foo', { configurable: true, value: 'baz' }), TypeError); + + assert.equal(obj.foo, 'bar'); }); TestClass.test('defining an enumerable property', function(assert, factory) { @@ -263,9 +299,15 @@ classes.forEach(TestClass => { let source = factory.source(); - assert.throws(() => source.foo = 'baz', TypeError); + if (hasCompliantAssign) { + assert.throws(() => source.foo = 'baz', TypeError); + assert.throws(() => obj.foo = 'baz', TypeError); + } else { + source.foo = 'baz'; + obj.foo = 'baz'; + } - assert.throws(() => obj.foo = 'baz', TypeError); + assert.equal(obj.foo, 'bar'); }); TestClass.test('defining a getter', function(assert, factory) { @@ -347,6 +389,12 @@ classes.forEach(TestClass => { assert.equal(obj.fooBar, 'FOO-BAR'); - assert.throws(() => obj.fooBar = 'foobar', TypeError); + if (hasCompliantAssign) { + assert.throws(() => obj.fooBar = 'foobar', TypeError); + } else { + obj.fooBar = 'foobar'; + } + + assert.equal(obj.fooBar, 'FOO-BAR'); }); });