Merge pull request #15177 from rwjblue/take-two-dont-mutate-own-prope…

…rties

[BUGFIX beta] Avoid creating enumerable properties on all objects created by DI.

packages/container/lib/container.js

@@ -418,6 +418,8 @@ function deprecatedFactoryFor(container, fullName, options = {}) {
     let cacheable = !areInjectionsDynamic(injections) && !areInjectionsDynamic(factoryInjections);
 
     factoryInjections[NAME_KEY] = registry.makeToString(factory, fullName);
+    injections._debugContainerKey = fullName;
+    setOwner(injections, container.owner);
 
     let injectedFactory = factory.extend(injections);
 
@@ -443,9 +445,6 @@ function injectionsFor(container, fullName) {
   let type = splitName[0];
 
   let injections = buildInjections(container, registry.getTypeInjections(type), registry.getInjections(fullName));
-  injections._debugContainerKey = fullName;
-
-  setOwner(injections, container.owner);
 
   return injections;
 }
@@ -488,6 +487,7 @@ function instantiate(factory, props, container, fullName) {
       // to create time injections
       // TODO: support new'ing for instantiation and merge injections for pure JS Functions
       let injections = injectionsFor(container, fullName);
+      injections._debugContainerKey = fullName;
 
       // Ensure that a container is available to an object during instantiation.
       // TODO - remove when Ember reaches v3.0.0
@@ -613,13 +613,22 @@ class DeprecatedFactoryManager {
 class FactoryManager {
   constructor(container, factory, fullName, normalizedName) {
     this.container = container;
+    this.owner = container.owner;
     this.class = factory;
     this.fullName = fullName;
     this.normalizedName = normalizedName;
     this.madeToString = undefined;
     this.injections = undefined;
   }
 
+  toString() {
+    if (!this.madeToString) {
+      this.madeToString = this.container.registry.makeToString(this.class, this.fullName);
+    }
+
+    return this.madeToString;
+  }
+
   create(options = {}) {
 
     let injections = this.injections;
@@ -631,7 +640,6 @@ class FactoryManager {
     }
     let props = assign({}, injections, options);
 
-    props[NAME_KEY] = this.madeToString || (this.madeToString = this.container.registry.makeToString(this.class, this.fullName));
 
     if (DEBUG) {
       let lazyInjections;
@@ -656,6 +664,21 @@ class FactoryManager {
       injectDeprecatedContainer(prototype, this.container);
     }
 
+    // required to allow access to things like
+    // the customized toString, _debugContainerKey,
+    // owner, etc. without a double extend and without
+    // modifying the objects properties
+    if (typeof this.class._initFactory === 'function') {
+      this.class._initFactory(this);
+    } else {
+      // in the non-Ember.Object case we need to still setOwner
+      // this is required for supporting glimmer environment and
+      // template instantiation which rely heavily on
+      // `options[OWNER]` being passed into `create`
+      // TODO: clean this up, and remove in future versions
+      setOwner(props, this.owner);
+    }
+
     return this.class.create(props);
   }
 }

packages/container/tests/container_test.js

@@ -1,4 +1,4 @@
-import { getOwner, OWNER } from 'ember-utils';
+import { getOwner, OWNER, assign } from 'ember-utils';
 import { ENV } from 'ember-environment';
 import { get } from 'ember-metal';
 import { Registry } from '..';
@@ -71,7 +71,7 @@ QUnit.test('A factory returned from lookupFactory has a debugkey', function() {
   equal(PostFactory._debugContainerKey, 'controller:post', 'factory instance receives _debugContainerKey');
 });
 
-QUnit.test('fallback for to create time injections if factory has no extend', function() {
+QUnit.test('uses create time injections if factory has no extend', function() {
   let registry = new Registry();
   let container = registry.container();
   let AppleController = factory();
@@ -85,7 +85,6 @@ QUnit.test('fallback for to create time injections if factory has no extend', fu
 
   let postController = container.lookup('controller:post');
 
-  equal(postController._debugContainerKey, 'controller:post', 'instance receives _debugContainerKey');
   ok(postController.apple instanceof AppleController, 'instance receives an apple of instance AppleController');
 });
 
@@ -157,9 +156,6 @@ QUnit.test('An individual factory with a registered injection receives the injec
   let postController = container.lookup('controller:post');
   let store = container.lookup('store:main');
 
-  equal(store._debugContainerKey, 'store:main');
-
-  equal(postController._debugContainerKey, 'controller:post');
   equal(postController.store, store, 'has the correct store injected');
 });
 
@@ -742,3 +738,52 @@ QUnit.test('#factoryFor options passed to create clobber injections', (assert) =
 
   assert.equal(instrance.ajax, 'fetch');
 });
+
+QUnit.test('#factoryFor does not add properties to the object being instantiated when _initFactory is present', function(assert) {
+  let owner = {};
+  let registry = new Registry();
+  let container = registry.container();
+
+  let factory;
+  class Component {
+    static _initFactory(_factory) { factory = _factory; }
+    static create(options) {
+      let instance = new this();
+      assign(instance, options);
+      return instance;
+    }
+  }
+  registry.register('component:foo-bar', Component);
+
+  let componentFactory = container.factoryFor('component:foo-bar');
+  let instance = componentFactory.create();
+
+  // note: _guid and isDestroyed are being set in the `factory` constructor
+  // not via registry/container shenanigans
+  assert.deepEqual(Object.keys(instance), []);
+});
+
+// this is skipped until templates and the glimmer environment do not require `OWNER` to be
+// passed in as constructor args
+QUnit.skip('#factoryFor does not add properties to the object being instantiated', function(assert) {
+  let owner = {};
+  let registry = new Registry();
+  let container = registry.container();
+
+  let factory;
+  class Component {
+    static create(options) {
+      let instance = new this();
+      assign(instance, options);
+      return instance;
+    }
+  }
+  registry.register('component:foo-bar', Component);
+
+  let componentFactory = container.factoryFor('component:foo-bar');
+  let instance = componentFactory.create();
+
+  // note: _guid and isDestroyed are being set in the `factory` constructor
+  // not via registry/container shenanigans
+  assert.deepEqual(Object.keys(instance), []);
+});

packages/ember-metal/lib/meta.js

@@ -98,6 +98,7 @@ export class Meta {
     this._chains = undefined;
     this._tag = undefined;
     this._tags = undefined;
+    this._factory = undefined;
 
     // initial value for all flags right now is false
     // see FLAGS const for detailed list of flags used
@@ -345,6 +346,14 @@ export class Meta {
       obj[key] = value;
     }
   }
+
+  set factory(factory) {
+    this._factory = factory;
+  }
+
+  get factory() {
+    return this._factory;
+  }
 }
 
 const NODE_STACK = [];

packages/ember-runtime/lib/system/core_object.js

@@ -52,7 +52,7 @@ function makeCtor() {
   // possible.
 
   let wasApplied = false;
-  let initProperties;
+  let initProperties, initFactory;
 
   class Class {
     constructor() {
@@ -68,6 +68,11 @@ function makeCtor() {
       let m = meta(this);
       let proto = m.proto;
       m.proto = this;
+
+      if (initFactory) {
+        m.factory = initFactory;
+        initFactory = null;
+      }
       if (initProperties) {
         // capture locally so we can clear the closed over variable
         let props = initProperties;
@@ -180,6 +185,7 @@ function makeCtor() {
     }
 
     static _initProperties(args) { initProperties = args; }
+    static _initFactory(factory) { initFactory = factory; }
 
     static proto() {
       let superclass = Class.superclass;
@@ -540,7 +546,8 @@ CoreObject.PrototypeMixin = Mixin.create({
   toString() {
     let hasToStringExtension = typeof this.toStringExtension === 'function';
     let extension = hasToStringExtension ? `:${this.toStringExtension()}` : '';
-    let ret = `<${this[NAME_KEY] || this.constructor.toString()}:${guidFor(this)}${extension}>`;
+
+    let ret = `<${this[NAME_KEY] || meta(this).factory || this.constructor.toString()}:${guidFor(this)}${extension}>`;
 
     return ret;
   }

packages/ember-runtime/lib/system/object.js

@@ -3,13 +3,16 @@
 @submodule ember-runtime
 */
 
-import { symbol } from 'ember-utils';
-import { on } from 'ember-metal';
+import { symbol, NAME_KEY, OWNER } from 'ember-utils';
+import { on, descriptor, meta as metaFor } from 'ember-metal';
 import CoreObject from './core_object';
 import Observable from '../mixins/observable';
 import { assert } from 'ember-debug';
 import { DEBUG } from 'ember-env-flags';
 
+let OVERRIDE_CONTAINER_KEY = symbol('OVERRIDE_CONTAINER_KEY');
+let OVERRIDE_OWNER = symbol('OVERRIDE_OWNER');
+
 /**
   `Ember.Object` is the main base class for all Ember objects. It is a subclass
   of `Ember.CoreObject` with the `Ember.Observable` mixin applied. For details,
@@ -21,7 +24,48 @@ import { DEBUG } from 'ember-env-flags';
   @uses Ember.Observable
   @public
 */
-const EmberObject = CoreObject.extend(Observable);
+const EmberObject = CoreObject.extend(Observable, {
+  _debugContainerKey: descriptor({
+    enumerable: false,
+    get() {
+      if (this[OVERRIDE_CONTAINER_KEY]) {
+        return this[OVERRIDE_CONTAINER_KEY];
+      }
+
+      let meta = metaFor(this);
+      let { factory } = meta;
+
+      return factory && factory.fullName;
+    },
+
+    // we need a setter here largely to support the legacy
+    // `owner._lookupFactory` and its double extend
+    set(value) {
+      this[OVERRIDE_CONTAINER_KEY] = value;
+    }
+  }),
+
+  [OWNER]: descriptor({
+    enumerable: false,
+    get() {
+      if (this[OVERRIDE_OWNER]) {
+        return this[OVERRIDE_OWNER];
+      }
+
+      let meta = metaFor(this);
+      let { factory } = meta;
+
+      return factory && factory.owner;
+    },
+
+    // we need a setter here largely to support the legacy
+    // `owner._lookupFactory` and its double extend
+    set(value) {
+      this[OVERRIDE_OWNER] = value;
+    }
+  })
+});
+
 EmberObject.toString = () => 'Ember.Object';
 
 export let FrameworkObject = EmberObject;