-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Don’t share view registry across containers #10599
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would probably want to move these asserts to the _register function |
||
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... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like we don't need this line anymore |
||
|
||
if (!this.isVirtual) { | ||
delete View.views[view.elementId]; | ||
view._unregister(); | ||
} | ||
}, | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1305,6 +1305,10 @@ var View = CoreView.extend( | |
} | ||
|
||
this._super.apply(this, arguments); | ||
|
||
if (!this._viewRegistry) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Isn't this handled by the buildDefaultInstance? Why do we need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @igorT - Not all view creation goes through the container via a fully booted application (most specifically those created in tests). It is possible that we might not care if those end up in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Those tests should either then not require global Ember.View.views or need to mock it/require it. We can leave that for a later refactor though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @igorT - Yep, I agree. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rwjblue is correct, there are a ton of tests (and presumably apps) that create container-less Views. That's what this code is handling. |
||
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; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wouldn't it be simpler to do
EmberView.views = instance.container.lookup('-view-registry:main')