[FEATURE factory-for] Allow for incremental roll out factoryFor

The public factoryFor API should always return `class` as the
no-double-extend version. However internal APIs already refactored to
use FACTORY_FOR should use the double extended version if the
no-double-extend feature flag is false.

Author: mixonic

packages/container/lib/container.js

@@ -142,9 +142,11 @@ Container.prototype = {
   lookupFactory(fullName, options) {
     assert('fullName must be a proper full name', this.registry.validateFullName(fullName));
 
-    if (isFeatureEnabled('ember-no-double-extend')) {
-      deprecate('Using "_lookupFactory" is deprecated. Please use container.factoryFor instead.', false, { id: 'container-lookupFactory', until: '2.13.0', url: 'TODO' });
-    }
+    deprecate(
+      'Using "_lookupFactory" is deprecated. Please use container.factoryFor instead.',
+      !isFeatureEnabled('ember-factory-for'),
+      { id: 'container-lookupFactory', until: '2.13.0', url: 'TODO' }
+    );
 
     return deprecatedFactoryFor(this, this.registry.normalize(fullName), options);
   },
@@ -154,57 +156,27 @@ Container.prototype = {
     return deprecatedFactoryFor(this, this.registry.normalize(fullName), options);
   },
 
+  /*
+   * This internal version of factoryFor swaps between the public API for
+   * factoryFor (class is the registered class) and a transition implementation
+   * (class is the double-extended class). It is *not* the public API version
+   * of factoryFor, which always returns the registered class.
+   */
   [FACTORY_FOR](fullName, options = {}) {
-    let manager;
     if (isFeatureEnabled('ember-no-double-extend')) {
-      let normalizedName = this.registry.normalize(fullName);
-      assert('fullName must be a proper full name', this.registry.validateFullName(normalizedName));
-
-      if (options.source) {
-        normalizedName = this.registry.expandLocalLookup(fullName, options);
-        // if expandLocalLookup returns falsey, we do not support local lookup
-        if (!normalizedName) { return; }
+      if (isFeatureEnabled('ember-factory-for')) {
+        return this.factoryFor(fullName, options);
+      } else {
+        /* This throws in case of a poorly designed build */
+        throw new Error('If ember-no-double-extend is enabled, ember-factory-for must also be enabled');
       }
-
-      let factory = this.registry.resolve(normalizedName);
-
-      if (factory === undefined) { return; }
-
-      manager = new FactoryManager(this, factory, fullName, normalizedName);
-    } else {
-      let factory = this.lookupFactory(fullName, options);
-      if (factory === undefined) { return; }
-      manager = new DeprecatedFactoryManager(this, factory, fullName);
     }
+    let factory = this.lookupFactory(fullName, options);
+    if (factory === undefined) { return; }
+    let manager = new DeprecatedFactoryManager(this, factory, fullName);
 
     runInDebug(() => {
-      if (HAS_PROXY) {
-        let validator = {
-          get(obj, prop) {
-            if (prop !== 'class' && prop !== 'create') {
-              throw new Error(`You attempted to access "${prop}" on a factory manager created by container#factoryFor. "${prop}" is not a member of a factory manager."`);
-            }
-
-            return obj[prop];
-          },
-          set(obj, prop, value) {
-            throw new Error(`You attempted to set "${prop}" on a factory manager created by container#factoryFor. A factory manager is a read-only construct.`);
-          }
-        };
-
-        // Note:
-        // We have to proxy access to the manager here so that private property
-        // access doesn't cause the above errors to occur.
-        let m = manager;
-        let proxiedManager = {
-          class: m.class,
-          create(props) {
-            return m.create(props);
-          }
-        };
-
-        manager = new Proxy(proxiedManager, validator);
-      }
+      manager = wrapManagerInDeprecationProxy(manager);
     });
 
     return manager;
@@ -255,10 +227,46 @@ Container.prototype = {
   }
 };
 
+/*
+ * Wrap a factory manager in a proxy which will not permit properties to be
+ * set on the manager.
+ */
+function wrapManagerInDeprecationProxy(manager) {
+  if (HAS_PROXY) {
+    let validator = {
+      get(obj, prop) {
+        if (prop !== 'class' && prop !== 'create') {
+          throw new Error(`You attempted to access "${prop}" on a factory manager created by container#factoryFor. "${prop}" is not a member of a factory manager."`);
+        }
+
+        return obj[prop];
+      },
+      set(obj, prop, value) {
+        throw new Error(`You attempted to set "${prop}" on a factory manager created by container#factoryFor. A factory manager is a read-only construct.`);
+      }
+    };
+
+    // Note:
+    // We have to proxy access to the manager here so that private property
+    // access doesn't cause the above errors to occur.
+    let m = manager;
+    let proxiedManager = {
+      class: m.class,
+      create(props) {
+        return m.create(props);
+      }
+    };
+
+    return new Proxy(proxiedManager, validator);
+  }
+}
+
 if (isFeatureEnabled('ember-factory-for')) {
   /**
-   Given a fullName, return the corresponding factory. The consumer of factory is repsonsible
-   for the destruction of the factory as there is no way to understand the objects lifecycle.
+   Given a fullName, return the corresponding factory. The consumer of the factory
+   is responsible for the destruction of any factory instances, as there is no
+   way for the container to ensure instances are destroyed when it itself is
+   destroyed.
 
    @public
    @method factoryFor
@@ -267,8 +275,27 @@ if (isFeatureEnabled('ember-factory-for')) {
    @param {String} [options.source] The fullname of the request source (used for local lookup)
    @return {any}
    */
-  Container.prototype.factoryFor = function _factoryFor() {
-    return this[FACTORY_FOR](...arguments);
+  Container.prototype.factoryFor = function _factoryFor(fullName, options = {}) {
+    let normalizedName = this.registry.normalize(fullName);
+    assert('fullName must be a proper full name', this.registry.validateFullName(normalizedName));
+
+    if (options.source) {
+      normalizedName = this.registry.expandLocalLookup(fullName, options);
+      // if expandLocalLookup returns falsey, we do not support local lookup
+      if (!normalizedName) { return; }
+    }
+
+    let factory = this.registry.resolve(normalizedName);
+
+    if (factory === undefined) { return; }
+
+    let manager = new FactoryManager(this, factory, fullName, normalizedName);
+
+    runInDebug(() => {
+      manager = wrapManagerInDeprecationProxy(manager);
+    });
+
+    return manager;
   };
 }
 

packages/container/tests/container_test.js

@@ -3,6 +3,7 @@ import { ENV } from 'ember-environment';
 import { get, isFeatureEnabled } from 'ember-metal';
 import { Registry } from '../index';
 import { factory } from 'internal-test-helpers';
+import { FACTORY_FOR } from 'container';
 
 let originalModelInjections;
 
@@ -18,7 +19,7 @@ QUnit.module('Container', {
 function lookupFactory(name, container, options) {
   let factory;
   if (isFeatureEnabled('ember-no-double-extend')) {
-    expectDeprecation(() => {
+    ignoreDeprecation(() => {
       factory = container.lookupFactory(name, options);
     });
   } else {
@@ -489,11 +490,7 @@ QUnit.test('factory for non extendables resolves are cached', function() {
 });
 
 QUnit.test('The `_onLookup` hook is called on factories when looked up the first time', function() {
-  if (isFeatureEnabled('ember-factory-for') && isFeatureEnabled('ember-no-double-extend')) {
-    expect(4);
-  } else {
-    expect(2);
-  }
+  expect(2);
 
   let registry = new Registry();
   let container = registry.container();
@@ -701,6 +698,20 @@ QUnit.test('lookup passes options through to expandlocallookup', function(assert
   assert.ok(PostControllerLookupResult instanceof PostController);
 });
 
+QUnit.test('#[FACTORY_FOR] class is the injected factory', (assert) => {
+  let registry = new Registry();
+  let container = registry.container();
+
+  let Component = factory();
+  registry.register('component:foo-bar', Component);
+
+  let factoryCreator = container[FACTORY_FOR]('component:foo-bar');
+  if (isFeatureEnabled('ember-no-double-extend')) {
+    assert.deepEqual(factoryCreator.class, Component, 'No double extend');
+  } else {
+    assert.deepEqual(factoryCreator.class, container.lookupFactory('component:foo-bar'), 'Double extended class');
+  }
+});
 
 if (isFeatureEnabled('ember-factory-for')) {
   QUnit.test('#factoryFor must supply a fullname', (assert) => {
@@ -731,11 +742,7 @@ if (isFeatureEnabled('ember-factory-for')) {
     registry.register('component:foo-bar', Component);
 
     let factoryCreator = container.factoryFor('component:foo-bar');
-    if (isFeatureEnabled('ember-no-double-extend')) {
-      assert.deepEqual(factoryCreator.class, Component, 'No double extend');
-    } else {
-      assert.notDeepEqual(factoryCreator.class, Component, 'Double extended class');
-    }
+    assert.deepEqual(factoryCreator.class, Component, 'No double extend');
   });
 
   QUnit.test('#factoryFor instance have a common parent', (assert) => {

packages/ember-application/tests/system/dependency_injection/default_resolver_test.js

@@ -65,14 +65,16 @@ QUnit.test('the default resolver looks up templates in Ember.TEMPLATES', functio
   setTemplate('fooBar', fooBarTemplate);
   setTemplate('fooBar/baz', fooBarBazTemplate);
 
+  ignoreDeprecation(() => {
+    equal(locator.lookupFactory('template:foo'), fooTemplate, 'resolves template:foo');
+    equal(locator.lookupFactory('template:fooBar'), fooBarTemplate, 'resolves template:foo_bar');
+    equal(locator.lookupFactory('template:fooBar.baz'), fooBarBazTemplate, 'resolves template:foo_bar.baz');
+  });
+
   if (isFeatureEnabled('ember-factory-for')) {
     equal(locator.factoryFor('template:foo').class, fooTemplate, 'resolves template:foo');
     equal(locator.factoryFor('template:fooBar').class, fooBarTemplate, 'resolves template:foo_bar');
     equal(locator.factoryFor('template:fooBar.baz').class, fooBarBazTemplate, 'resolves template:foo_bar.baz');
-  } else {
-    equal(locator.lookupFactory('template:foo'), fooTemplate, 'resolves template:foo');
-    equal(locator.lookupFactory('template:fooBar'), fooBarTemplate, 'resolves template:foo_bar');
-    equal(locator.lookupFactory('template:fooBar.baz'), fooBarBazTemplate, 'resolves template:foo_bar.baz');
   }
 });
 
@@ -93,48 +95,61 @@ QUnit.test('the default resolver looks up arbitrary types on the namespace', fun
 QUnit.test('the default resolver resolves models on the namespace', function() {
   application.Post = EmberObject.extend({});
 
+  ignoreDeprecation(() => {
+    detectEqual(application.Post, locator.lookupFactory('model:post'), 'looks up Post model on application');
+  });
   if (isFeatureEnabled('ember-factory-for')) {
     detectEqual(application.Post, locator.factoryFor('model:post').class, 'looks up Post model on application');
-  } else {
-    detectEqual(application.Post, locator.lookupFactory('model:post'), 'looks up Post model on application');
   }
 });
 
 QUnit.test('the default resolver resolves *:main on the namespace', function() {
   application.FooBar = EmberObject.extend({});
 
+  ignoreDeprecation(() => {
+    detectEqual(application.FooBar, locator.lookupFactory('foo-bar:main'), 'looks up FooBar type without name on application');
+  });
   if (isFeatureEnabled('ember-factory-for')) {
     detectEqual(application.FooBar, locator.factoryFor('foo-bar:main').class, 'looks up FooBar type without name on application');
-  } else {
-    detectEqual(application.FooBar, locator.lookupFactory('foo-bar:main'), 'looks up FooBar type without name on application');
   }
 });
 
-QUnit.test('the default resolver resolves container-registered helpers', function() {
+if (isFeatureEnabled('ember-factory-for')) {
+  QUnit.test('the default resolver resolves container-registered helpers', function() {
+    let shorthandHelper = makeHelper(() => {});
+    let helper = Helper.extend();
+
+    application.register('helper:shorthand', shorthandHelper);
+    application.register('helper:complete', helper);
+
+    let lookedUpShorthandHelper = locator.factoryFor('helper:shorthand').class;
+
+    ok(lookedUpShorthandHelper.isHelperInstance, 'shorthand helper isHelper');
+
+    let lookedUpHelper = locator.factoryFor('helper:complete').class;
+
+    ok(lookedUpHelper.isHelperFactory, 'complete helper is factory');
+    ok(helper.detect(lookedUpHelper), 'looked up complete helper');
+  });
+}
+
+QUnit.test('the default resolver resolves container-registered helpers via lookupFor', function() {
   let shorthandHelper = makeHelper(() => {});
   let helper = Helper.extend();
 
   application.register('helper:shorthand', shorthandHelper);
   application.register('helper:complete', helper);
 
-  let lookedUpShorthandHelper;
-  if (isFeatureEnabled('ember-factory-for')) {
-    lookedUpShorthandHelper = locator.factoryFor('helper:shorthand').class;
-  } else {
-    lookedUpShorthandHelper = locator.lookupFactory('helper:shorthand');
-  }
+  ignoreDeprecation(() => {
+    let lookedUpShorthandHelper = locator.lookupFactory('helper:shorthand');
 
-  ok(lookedUpShorthandHelper.isHelperInstance, 'shorthand helper isHelper');
+    ok(lookedUpShorthandHelper.isHelperInstance, 'shorthand helper isHelper');
 
-  let lookedUpHelper;
-  if (isFeatureEnabled('ember-factory-for')) {
-    lookedUpHelper = locator.factoryFor('helper:complete').class;
-  } else {
-    lookedUpHelper = locator.lookupFactory('helper:complete');
-  }
+    let lookedUpHelper = locator.lookupFactory('helper:complete');
 
-  ok(lookedUpHelper.isHelperFactory, 'complete helper is factory');
-  ok(helper.detect(lookedUpHelper), 'looked up complete helper');
+    ok(lookedUpHelper.isHelperFactory, 'complete helper is factory');
+    ok(helper.detect(lookedUpHelper), 'looked up complete helper');
+  });
 });
 
 QUnit.test('the default resolver resolves helpers on the namespace', function() {

packages/ember-routing/tests/system/controller_for_test.js

@@ -7,6 +7,7 @@ import {
 import controllerFor from '../../system/controller_for';
 import generateController from '../../system/generate_controller';
 import { buildOwner } from 'internal-test-helpers';
+import { isFeatureEnabled } from 'ember-metal';
 
 function buildInstance(namespace) {
   let owner = buildOwner();
@@ -75,18 +76,33 @@ QUnit.module('Ember.generateController', {
   }
 });
 
-QUnit.test('generateController should create Ember.Controller', function() {
+QUnit.test('generateController should return Ember.Controller', function() {
   let controller = generateController(appInstance, 'home');
 
-  ok(controller instanceof Controller, 'should create controller');
+  ok(controller instanceof Controller, 'should return controller');
 });
 
 
-QUnit.test('generateController should create App.Controller if provided', function() {
+QUnit.test('generateController should return App.Controller if provided', function() {
   let controller;
   namespace.Controller = Controller.extend();
 
   controller = generateController(appInstance, 'home');
 
-  ok(controller instanceof namespace.Controller, 'should create controller');
+  ok(controller instanceof namespace.Controller, 'should return controller');
+});
+
+QUnit.test('generateController should return controller:basic if provided', function() {
+  let controller;
+
+  let BasicController = Controller.extend();
+  appInstance.register('controller:basic', BasicController);
+
+  controller = generateController(appInstance, 'home');
+
+  if (isFeatureEnabled('ember-no-double-extend')) {
+    ok(controller instanceof BasicController, 'should return base class of controller');
+  } else {
+    ok(controller instanceof appInstance._lookupFactory('controller:basic'), 'should return double-extended controller');
+  }
 });