[FEATURE factory-for] Implement factoryFor #14360

chadhietala · 2016-09-26T22:18:49Z

Implements lookup.factoryFor to avoid double extending all objects.

rwjblue · 2016-09-26T22:24:58Z

rwjblue · 2016-09-28T00:49:36Z

Haven't had a chance to review in detail, but I did notice that this still needs feature flagging.

chadhietala · 2016-09-28T05:03:43Z

@stefanpenner I think this is good for first round of review. Currently no feature flagging has been done because these are effectively internal changes with no user-land implications.

One outstanding thing is if we need to inject the deprecated container in this new public API.

rwjblue · 2016-09-28T13:13:55Z

Currently no feature flagging has been done because these are effectively internal changes with no user-land implications.

O_o, isn't the whole point of the RFC to expose this as public API?

mixonic · 2016-09-28T17:05:42Z

@chadhietala

inject the deprecated container in this new public API

I'm not quite sure what this means, the container is not deprecated. Will review here.

homu · 2016-09-30T20:25:59Z

☔ The latest upstream changes (presumably #14355) made this pull request unmergeable. Please resolve the merge conflicts.

homu · 2016-10-06T18:00:37Z

☔ The latest upstream changes (presumably #14428) made this pull request unmergeable. Please resolve the merge conflicts.

mixonic

Thanks so much for getting this going @chadhietala. The big item is that lookup should still be using double extend. If you want to add a second feature flag (ideally in a followup) that experimentally switches lookup to the non-double-extended factory, that would definitely be welcome as a distinct feature flag.

mixonic · 2016-09-28T17:15:35Z

packages/ember-extension-support/lib/data_adapter.js

@@ -163,7 +163,7 @@ export default EmberObject.extend({

  _nameToClass(type) {
    if (typeof type === 'string') {
-      type = getOwner(this)._lookupFactory(`model:${type}`);
+      type = getOwner(this).factoryFor(`model:${type}`).class;


There is a logic change happening here- Where the old version was returning the double-extended class now the registered class is being returned. How does that impact the logic?

I actually do not understand how the type is used here. It's passed around all over the place, but isn't used for any of the output.

Hm seems like backwards-compat for pre-1.13 per this hook implementation. Super unfortunate that any of the class stuff is still used that way. Seems like it needs a deprecation cycle in ember-data.

It seems there is some use of the types in Ember-Data. For example here attributes are read off the class. Removing the double-extend means the attributes CP would be called and memoized on the base class, and thus shared between test runs etc.

This is definitely a change, described in the design section.

Some other examples of libraries this would impact are listed in the drawbacks section.

mixonic · 2016-09-28T17:16:21Z

packages/ember-glimmer/lib/environment.js

@@ -75,7 +75,8 @@ export default class Environment extends GlimmerEnvironment {

    this._definitionCache = new Cache(2000, ({ name, source, owner }) => {
      let { component: ComponentClass, layout } = lookupComponent(owner, name, { source });


ComponentClass seems a poor name now. componentFactory if you intended to pass the whole thing through instead of just the class?

mixonic · 2016-10-16T05:35:33Z

packages/container/lib/container.js

+
+    let manager = {
+      class: factory,
+      create(options = {}) {


I expected the implementation here to be

let factory = this._lookupFactory(name); return factory.create(options);

for the initial pass of factoryFor create per https://github.com/emberjs/rfcs/blob/master/text/0000-factoryFor.md#detailed-design. Additionally https://github.com/emberjs/rfcs/blob/master/text/0000-factoryFor.md#development-mode-proxy. In practice literally implementing as _lookupFactory(name).create(props) might not work, but it seems like it should and would preserve the double-extend behavior during instance creation. We should decide if this is really needed, will raise it in some conversations.

Not sure if I follow. Can we not just transition all the internal stuff over to factoryFor?

The intent was to encourage libs/apps to use the new API and drop their current reliance on the double-extended class, then change over internals and any other small breaking changes that will happen after most apps have refactored toward factoryFor and stopped expecting a double-extend.

mixonic · 2016-10-16T05:47:17Z

packages/container/lib/container.js

-  if (isSingleton(container, fullName) && options.singleton !== false) {
-    container.cache[fullName] = value;
+function isFactoryClass(container, fullname, { instantiate, singleton }) {
+  return (!isSingleton(container, fullname) || singleton === false) && (!shouldInstantiate(container, fullname) && instantiate === false);


putting the faster checks first seems ideal:

function isFactoryClass(container, fullname, { instantiate, singleton }) { return ( (singleton === false || !isSingleton(container, fullname)) && (instantiate === false || !shouldInstantiate(container, fullname)) ); }

thus avoiding the function calls in many cases.

mixonic · 2016-10-16T05:48:32Z

packages/container/lib/container.js

+    if (isFeatureEnabled('container-factoryFor')) {
+      deprecate('Using "_lookupFactory" is deprecated. Please use container.factoryFor instead.', false, { id: 'container-lookupFactory', until: '2.12.0', url: 'TODO' });
+    }
+
    return factoryFor(this, this.registry.normalize(fullName), options);


can this function be renamed now, since it is not the factoryFor one would normally be looking for here.

mixonic · 2016-10-16T05:51:46Z

packages/container/lib/container.js

@@ -191,15 +280,47 @@ function lookup(container, fullName, options = {}) {
    return container.cache[fullName];
  }

-  let value = instantiate(container, fullName);
+  return instantiateFactory(container, fullName, options);


So this meanscontainer.lookup is now based on the the non-double-extended factory? This seems like the behavior we didn't intend to change until _lookupFactory is removed.

yes, we should only have 1 behavior in the system at any point in time, once we cut over it should be all no double extend.

mixonic · 2016-10-16T05:52:16Z

packages/container/lib/container.js

@@ -127,9 +135,86 @@ Container.prototype = {
   */
  lookupFactory(fullName, options) {
    assert('fullName must be a proper full name', this.registry.validateFullName(fullName));
+
+    if (isFeatureEnabled('container-factoryFor')) {
+      deprecate('Using "_lookupFactory" is deprecated. Please use container.factoryFor instead.', false, { id: 'container-lookupFactory', until: '2.12.0', url: 'TODO' });


Must be until: '2.13'. 2.12 is an LTS release and must contain this deprecation.

mixonic · 2016-10-16T05:54:02Z

packages/container/tests/container_test.js

+function lookupFactory(name, container, options) {
+  let factory;
+  if (isFeatureEnabled('container-factoryFor')) {
+    expectDeprecation(() => {


There is also an ignoreDeprecation which might be appropriate/useful here. Example:

ember.js/packages/ember-runtime/tests/controllers/controller_test.js

Line 118 in aeeddf7

ignoreDeprecation(function() {

mixonic · 2016-10-16T05:55:26Z

packages/container/tests/container_test.js

@@ -561,7 +581,8 @@ QUnit.test('A deprecated `container` property is appended to every object instan
  }, 'Using the injected `container` is deprecated. Please use the `getOwner` helper instead to access the owner of this object.');
 });

-QUnit.test('A deprecated `container` property is appended to every object instantiated from a non-extendable factory, and a fake container is available during instantiation.', function() {
+// This is testing that container was passed as an option
+QUnit.skip('A deprecated `container` property is appended to every object instantiated from a non-extendable factory, and a fake container is available during instantiation.', function() {


This test shows why we need to keep the "double extend" the default path for lookup until at least 2.13.

mixonic · 2016-10-16T05:58:47Z

packages/container/lib/container.js

+            throw new Error(`You attempted to set "${prop}" on a factory manager created by container#factoryFor. A factory manager is a read-only construct.`);
+          }
+        };
+        manager = new Proxy(manager, validator);


Would like to have tests included that:

getting a property that is not class or create throws an error

setting a property throws an error

calling instanceof with the return value from factoryFor throws an error

Per: https://github.com/emberjs/rfcs/blob/master/text/0000-factoryFor.md#development-mode-proxy

homu · 2016-10-17T22:35:15Z

☔ The latest upstream changes (presumably #14487) made this pull request unmergeable. Please resolve the merge conflicts.

chadhietala · 2016-10-25T03:32:09Z

I made some changes to this, but likely needs further guidance.

homu · 2016-10-30T00:55:08Z

☔ The latest upstream changes (presumably #14545) made this pull request unmergeable. Please resolve the merge conflicts.

chadhietala · 2016-12-18T03:35:12Z

Quick profiles from Travis-Web. This is consistent across runs. Looked at the homepage and build status pages. Benefits are bound to number of objects created.

Before: (Prod / Feature Flags Off)

After: (Prod / Feature Flags On)
About 100ms reduction

About 200ms reduction

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.

rwjblue · 2016-12-18T14:01:31Z

@chadhietala - I assume the After: (Prod / Feature Flags On) screenshots above are with both ember-factory-for and ember-no-double-extend flags on, is that right? Can you test with only ember-factory-for (I'm mostly looking to try to figure out if we will have temporary regressions in 2.12)?

rwjblue · 2016-12-18T14:30:05Z

packages/ember-application/lib/system/engine-instance.js

@@ -194,9 +194,25 @@ const EngineInstance = EmberObject.extend(RegistryProxyMixin, ContainerProxyMixi

    this.inject('view', '_environment', '-environment:main');
    this.inject('route', '_environment', '-environment:main');
+  },
+
+  [FACTORY_FOR](fullName, options) {


Lets make this part of the ContainerProxyMixin, this would simplify issues in ember-test-helpers land and actually keep the logic in the correct place (currently .lookup and ._lookupFactory are defined in ContainerProxyMixin so that seems like the logical place).

rwjblue · 2016-12-18T14:30:56Z

packages/ember-application/lib/system/engine-instance.js

  }
 });

+if (isFeatureEnabled('ember-factory-for')) {
+  EngineInstance.reopen({
+    factoryFor(fullName, options) {


This can be moved to the mixin also...

rwjblue · 2016-12-18T14:41:06Z

packages/container/lib/container.js


 const CONTAINER_OVERRIDE = symbol('CONTAINER_OVERRIDE');
+const HAS_PROXY = typeof Proxy === 'function';


Can we move this into ember-utils and export as HAS_NATIVE_PROXY? Very similar to what was done in #14649 for packages/ember-utils/lib/weak-map-utils.js.

rwjblue · 2016-12-18T14:44:01Z

tests/node/helpers/build-owner.js

@@ -1,5 +1,14 @@
 module.exports = function buildOwner(Ember, resolver) {
-  var Owner = Ember.Object.extend(Ember._RegistryProxyMixin, Ember._ContainerProxyMixin);
+  var FACTORY_FOR = Ember.Container.__FACTORY_FOR__;


See comments in #14360 (review) and emberjs/ember-test-helpers#191, we should be able to remove this.

I've made Robert's final requested change

mixonic · 2016-12-18T22:25:00Z

I've done some benchmarks against todomvc. I would expect the impact on a larger app to be much more significant (as @chadhietala's profiling shows).

This PR may have a minor performance regression:

The ember-factory-for feature flag does not seem to have a significant impact on performance (which is expected).

However the ember-no-double-extend feature flag makes a significant improvement.

I think we can continue to explore issues around backwards compatibility, performance, and the migration path to factoryFor on master. Merging this.

@chadhietala Thanks much for your patience and push on this gnarly, complex, cross-cutting part of Ember's API. The payoff (especially in big apps) is going to be really notable.

stefanpenner · 2016-12-19T02:15:50Z

👍

sandstrom · 2016-12-19T21:56:22Z

@chadhietala Awesome! Nice to be able to move into a public API for this + great performance improvements! ⛵️

chadhietala force-pushed the double-extend branch from fed86bf to ef5301e Compare September 26, 2016 22:19

chadhietala force-pushed the double-extend branch 3 times, most recently from c89e3fd to 6d1d689 Compare September 28, 2016 05:00

chadhietala force-pushed the double-extend branch from 6d1d689 to b732ef5 Compare September 28, 2016 05:17

chadhietala force-pushed the double-extend branch 6 times, most recently from 0d270ce to d649c52 Compare October 4, 2016 21:47

chadhietala mentioned this pull request Oct 7, 2016

factoryFor as a public API replacing _lookupFactory emberjs/rfcs#150

Merged

chadhietala force-pushed the double-extend branch 3 times, most recently from c46fd47 to f3ecc62 Compare October 15, 2016 19:44

chadhietala changed the title ~~[WIP] Feature factoryFor~~ [Feature] FactoryFor Oct 16, 2016

mixonic requested changes Oct 16, 2016

View reviewed changes

chadhietala force-pushed the double-extend branch 3 times, most recently from c993e4b to f6dfba0 Compare October 24, 2016 20:37

chadhietala and others added 3 commits December 17, 2016 17:45

[FEATURE factory-for] Implement factoryFor

eeb9175

[FEATURE factory-for] Make helpers work with factoryFor

210a92d

[FEATURE factory-for] Introduce internal FACTORY_FOR symbol method

c05d241

chadhietala force-pushed the double-extend branch 2 times, most recently from 3a63382 to 71c9815 Compare December 18, 2016 02:45

chadhietala changed the title ~~[Feature] FactoryFor~~ [FEATURE factory-for] Implement factoryFor Dec 18, 2016

chadhietala mentioned this pull request Dec 18, 2016

Add support for factoryFor emberjs/ember-test-helpers#191

Closed

chadhietala force-pushed the double-extend branch from 71c9815 to bac3a08 Compare December 18, 2016 07:17

rwjblue previously requested changes Dec 18, 2016

View reviewed changes

rwjblue reviewed Dec 18, 2016

View reviewed changes

chadhietala and others added 2 commits December 18, 2016 09:52

[FEATURE factory-for] Add symboled methods to container proxy

54a5c15

[FEATURE factory-for] Only expose factoryFor w/ FF

aefb6ed

mixonic force-pushed the double-extend branch from 155e807 to aefb6ed Compare December 18, 2016 18:35

mixonic merged commit ef8f46e into emberjs:master Dec 18, 2016

homu mentioned this pull request Dec 18, 2016

[WIP] Router service #13868

Closed

chadhietala deleted the double-extend branch December 18, 2016 22:28

homu mentioned this pull request Dec 18, 2016

[FEATURE ember-metal-meta-destructors] #14602

Closed

GavinJoyce mentioned this pull request Dec 21, 2016

Improved backtracking re-render assertion message #14723

Merged

25 tasks

mixonic mentioned this pull request Jan 5, 2017

Compat with factoryFor (Ember 2.12ish) emberjs/data#4737

Closed

GavinJoyce mentioned this pull request Jan 18, 2017

add test verifying that registering an unknown service raises an exception #14850

Merged

stefanpenner mentioned this pull request Feb 13, 2017

depending on "model.constructor.modelName" sets model.constructor to undefined #14014

Closed

stefanpenner mentioned this pull request Feb 23, 2017

Remove MODEL_FACTORY_INJECTIONS #9500

Closed

		@@ -75,7 +75,8 @@ export default class Environment extends GlimmerEnvironment {

		this._definitionCache = new Cache(2000, ({ name, source, owner }) => {
		let { component: ComponentClass, layout } = lookupComponent(owner, name, { source });


		const CONTAINER_OVERRIDE = symbol('CONTAINER_OVERRIDE');
		const HAS_PROXY = typeof Proxy === 'function';

[FEATURE factory-for] Implement factoryFor #14360

[FEATURE factory-for] Implement factoryFor #14360

Conversation

chadhietala commented Sep 26, 2016 • edited Loading

rwjblue commented Sep 26, 2016

rwjblue commented Sep 28, 2016

chadhietala commented Sep 28, 2016 • edited Loading

rwjblue commented Sep 28, 2016

mixonic commented Sep 28, 2016

homu commented Sep 30, 2016

homu commented Oct 6, 2016

mixonic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

homu commented Oct 17, 2016

chadhietala commented Oct 25, 2016

homu commented Oct 30, 2016

chadhietala commented Dec 18, 2016 • edited Loading

rwjblue commented Dec 18, 2016

rwjblue Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mixonic commented Dec 18, 2016

stefanpenner commented Dec 19, 2016

sandstrom commented Dec 19, 2016 • edited Loading

chadhietala commented Sep 26, 2016 •

edited

Loading

chadhietala commented Sep 28, 2016 •

edited

Loading

chadhietala commented Dec 18, 2016 •

edited

Loading

rwjblue Dec 18, 2016 •

edited

Loading

sandstrom commented Dec 19, 2016 •

edited

Loading