From fef509a5f33c7a4cc708a95951073f5346e37a32 Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Mon, 9 Mar 2015 22:30:11 -0700 Subject: [PATCH 1/3] =?UTF-8?q?Don=E2=80=99t=20share=20view=20registry=20a?= =?UTF-8?q?cross=20containers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, all rendered views were inserted into the Ember.View.views global hash, indexed by their `elementId`. This hash is used by the EventDispatcher to locate views, and is also used by some advanced developers as a debugging aid. Unfortunately, using a global shared store of views is problematic in the FastBoot/application instance case, since multiple views with the same elementId can exist at the same time. This creates a race condition where the second view to be created will throw an exception, as Ember believes that a view with that element ID has already been created (normally an error in “single app” mode). This commit introduces the notion of a “view registry”, which is just a simple hash injected onto views and shared container-wide. Because each application instance in FastBoot has its own container, these views will naturally be isolated. To preserve backwards-compatibility, the default instance created when the app is booting normally (i.e., autoboot is `false`) injects the global Ember.View.views hash. We should consider removing this entirely in Ember 2.0, now that the Container pane of the Inspector gives developers easier access to registered views. Do note, however, that the Event Dispatcher still has a dependency on the global Ember.View.views, though it could be relatively easily refactored to also have the view registry injected. --- .../lib/system/application.js | 11 +++++ .../tests/system/visit_test.js | 42 +++++++++++++++++++ .../ember-views/lib/views/states/in_dom.js | 6 +-- packages/ember-views/lib/views/view.js | 30 +++++++++++++ 4 files changed, 86 insertions(+), 3 deletions(-) diff --git a/packages/ember-application/lib/system/application.js b/packages/ember-application/lib/system/application.js index c06c03427ae..a0c6346d3ba 100644 --- a/packages/ember-application/lib/system/application.js +++ b/packages/ember-application/lib/system/application.js @@ -331,6 +331,13 @@ var Application = Namespace.extend(DeferredMixin, { buildDefaultInstance: function() { var instance = this.buildInstance(); + // For the default instance only, set the view registry to the global + // Ember.View.views hash for backwards-compatibility. + var registry = instance.applicationRegistry; + registry.unregister('-view-registry:main'); + registry.register('-view-registry:main', EmberView.views); + registry.optionsForType('-view-registry', { instantiate: false }); + // TODO2.0: Legacy support for App.__container__ // and global methods on App that rely on a single, // default instance. @@ -1006,6 +1013,10 @@ Application.reopenClass({ registry.register('view:select', SelectView); registry.register('view:-outlet', OutletView); + registry.register('-view-registry:main', { create: function() { return {}; } }); + + registry.injection('view', '_viewRegistry', '-view-registry:main'); + registry.register('view:default', _MetamorphView); registry.register('view:toplevel', EmberView.extend()); diff --git a/packages/ember-application/tests/system/visit_test.js b/packages/ember-application/tests/system/visit_test.js index 16d28dc0ac0..57bb8fbda56 100644 --- a/packages/ember-application/tests/system/visit_test.js +++ b/packages/ember-application/tests/system/visit_test.js @@ -2,6 +2,7 @@ import run from "ember-metal/run_loop"; import Application from "ember-application/system/application"; import ApplicationInstance from "ember-application/system/application-instance"; import Router from "ember-routing/system/router"; +import View from "ember-views/views/view"; import compile from "ember-template-compiler/system/compile"; function createApplication() { @@ -82,4 +83,45 @@ if (Ember.FEATURES.isEnabled('ember-application-visit')) { assert.ok(false, "The visit() promise was rejected: " + error); }); }); + + QUnit.test("Views created via visit() are not added to the global views hash", function(assert) { + QUnit.expect(6); + QUnit.stop(); + + var app; + + run(function() { + app = createApplication(); + app.instanceInitializer({ + name: 'register-application-template', + initialize: function(app) { + app.registry.register('template:application', compile('

Hello world

{{view "child"}}')); + app.registry.register('view:application', View.extend({ + elementId: 'my-cool-app' + })); + app.registry.register('view:child', View.extend({ + elementId: 'child-view' + })); + } + }); + }); + + assert.equal(Ember.$('#qunit-fixture').children().length, 0, "there are no elements in the fixture element"); + + app.visit('/').then(function(instance) { + QUnit.start(); + assert.ok(instance instanceof ApplicationInstance, "promise is resolved with an ApplicationInstance"); + + run(instance.view, 'appendTo', '#qunit-fixture'); + assert.equal(Ember.$("#qunit-fixture > #my-cool-app h1").text(), "Hello world", "the application was rendered once the promise resolves"); + assert.strictEqual(View.views['my-cool-app'], undefined, "view was not registered globally"); + ok(instance.container.lookup('-view-registry:main')['my-cool-app'] instanceof View, "view was registered on the instance's view registry"); + ok(instance.container.lookup('-view-registry:main')['child-view'] instanceof View, "child view was registered on the instance's view registry"); + + instance.destroy(); + }, function(error) { + QUnit.start(); + assert.ok(false, "The visit() promise was rejected: " + error); + }); + }); } diff --git a/packages/ember-views/lib/views/states/in_dom.js b/packages/ember-views/lib/views/states/in_dom.js index 19c5b799e31..cc0b7c7cc44 100644 --- a/packages/ember-views/lib/views/states/in_dom.js +++ b/packages/ember-views/lib/views/states/in_dom.js @@ -21,8 +21,8 @@ merge(inDOM, { // Register the view for event handling. This hash is used by // Ember.EventDispatcher to dispatch incoming events. if (!view.isVirtual) { - Ember.assert("Attempted to register a view with an id already in use: "+view.elementId, !View.views[view.elementId]); - View.views[view.elementId] = view; + Ember.assert("Attempted to register a view with an id already in use: "+view.elementId, !view._viewRegistry[view.elementId]); + view._register(); } Ember.runInDebug(function() { @@ -36,7 +36,7 @@ merge(inDOM, { if (!View) { View = requireModule('ember-views/views/view')["default"]; } // ES6TODO: this sucks. Have to avoid cycles... if (!this.isVirtual) { - delete View.views[view.elementId]; + view._unregister(); } }, diff --git a/packages/ember-views/lib/views/view.js b/packages/ember-views/lib/views/view.js index 706b7272530..480c585d88e 100644 --- a/packages/ember-views/lib/views/view.js +++ b/packages/ember-views/lib/views/view.js @@ -1305,6 +1305,10 @@ var View = CoreView.extend( } this._super.apply(this, arguments); + + if (!this._viewRegistry) { + this._viewRegistry = View.views; + } }, __defineNonEnumerable: function(property) { @@ -1371,6 +1375,32 @@ var View = CoreView.extend( return this.currentState.handleEvent(this, eventName, evt); }, + /** + Registers the view in the view registry, keyed on the view's `elementId`. + This is used by the EventDispatcher to locate the view in response to + events. + + This method should only be called once the view has been inserted into the + DOM. + + @method _register + @private + */ + _register: function() { + this._viewRegistry[this.elementId] = this; + }, + + /** + Removes the view from the view registry. This should be called when the + view is removed from DOM. + + @method _unregister + @private + */ + _unregister: function() { + delete this._viewRegistry[this.elementId]; + }, + registerObserver: function(root, path, target, observer) { if (!observer && 'function' === typeof target) { observer = target; From 42993e5a4ba55cbb1919f1ce4108a4093d11ea89 Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Tue, 10 Mar 2015 10:36:32 -0700 Subject: [PATCH 2/3] Move view registry assertions into View --- packages/ember-views/lib/views/states/in_dom.js | 8 -------- packages/ember-views/lib/views/view.js | 1 + 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/packages/ember-views/lib/views/states/in_dom.js b/packages/ember-views/lib/views/states/in_dom.js index cc0b7c7cc44..bbdc6588fb0 100644 --- a/packages/ember-views/lib/views/states/in_dom.js +++ b/packages/ember-views/lib/views/states/in_dom.js @@ -1,4 +1,3 @@ -import Ember from "ember-metal/core"; // Ember.assert import create from 'ember-metal/platform/create'; import merge from "ember-metal/merge"; import EmberError from "ember-metal/error"; @@ -12,16 +11,11 @@ import hasElement from "ember-views/views/states/has_element"; var inDOM = create(hasElement); -var View; - merge(inDOM, { enter: function(view) { - if (!View) { View = requireModule('ember-views/views/view')["default"]; } // ES6TODO: this sucks. Have to avoid cycles... - // Register the view for event handling. This hash is used by // Ember.EventDispatcher to dispatch incoming events. if (!view.isVirtual) { - Ember.assert("Attempted to register a view with an id already in use: "+view.elementId, !view._viewRegistry[view.elementId]); view._register(); } @@ -33,8 +27,6 @@ merge(inDOM, { }, exit: function(view) { - if (!View) { View = requireModule('ember-views/views/view')["default"]; } // ES6TODO: this sucks. Have to avoid cycles... - if (!this.isVirtual) { view._unregister(); } diff --git a/packages/ember-views/lib/views/view.js b/packages/ember-views/lib/views/view.js index 480c585d88e..873a567b8ab 100644 --- a/packages/ember-views/lib/views/view.js +++ b/packages/ember-views/lib/views/view.js @@ -1387,6 +1387,7 @@ var View = CoreView.extend( @private */ _register: function() { + Ember.assert("Attempted to register a view with an id already in use: "+this.elementId, !this._viewRegistry[this.elementId]); this._viewRegistry[this.elementId] = this; }, From 6a71ff26042f2e6f9516f9616d88159a2acd8bec Mon Sep 17 00:00:00 2001 From: Tom Dale Date: Tue, 10 Mar 2015 10:42:11 -0700 Subject: [PATCH 3/3] Use less container trickery thanks @igorT --- packages/ember-application/lib/system/application.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/ember-application/lib/system/application.js b/packages/ember-application/lib/system/application.js index a0c6346d3ba..3bc29b4d604 100644 --- a/packages/ember-application/lib/system/application.js +++ b/packages/ember-application/lib/system/application.js @@ -333,10 +333,7 @@ var Application = Namespace.extend(DeferredMixin, { // For the default instance only, set the view registry to the global // Ember.View.views hash for backwards-compatibility. - var registry = instance.applicationRegistry; - registry.unregister('-view-registry:main'); - registry.register('-view-registry:main', EmberView.views); - registry.optionsForType('-view-registry', { instantiate: false }); + EmberView.views = instance.container.lookup('-view-registry:main'); // TODO2.0: Legacy support for App.__container__ // and global methods on App that rely on a single,