[BUGFIX canary] Fix _lookupFactory deprecation for Ember canary #4743

locks · 2017-01-10T17:09:19Z

No description provided.

bmac · 2017-01-11T20:45:47Z

addon/-private/system/store.js

+    if (owner.factoryFor) {
+      mixin = owner.factoryFor('mixin:' + normalizedModelName) && owner.factoryFor('mixin:' + normalizedModelName).class;
+    } else {
+      mixin = owner._lookupFactory('mixin:' + normalizedModelName);


Should this use ES6 backticks for consistency with the other uses?

bmac · 2017-01-11T20:47:11Z

addon/-private/utils.js

+  if (owner.factoryFor) {
+    return owner.factoryFor
+  }
+}


Is this uses?

fivetanley · 2017-01-11T23:35:56Z

addon/-private/utils.js

@@ -43,5 +43,6 @@ function getOwner(context) {

 export {
  modelHasAttributeOrRelationshipNamedType,
-  getOwner
+  getOwner,
+  factoryFor


this appears to not be a function anymore?

oops, I only removed part of the 💩

bmac · 2017-01-12T15:34:32Z

addon/-private/utils.js

@@ -12,11 +12,11 @@ function modelHasAttributeOrRelationshipNamedType(modelClass) {
  return get(modelClass, 'attributes').has('type') || get(modelClass, 'relationshipsByName').has('type');
 }

-/*
+/**


Do you mind removing the extra *s that were added in this file? YUIDoc sees these as API docs even though these functions are never made available to the user.

pangratz · 2017-01-12T15:38:10Z

addon/-private/system/store.js

+    let mixin;
+
+    if (owner.factoryFor) {
+      mixin = owner.factoryFor(`mixin:${normalizedModelName}`) && owner.factoryFor(`mixin:${normalizedModelName}`).class;


Should this be cached?

pangratz · 2017-01-12T15:39:08Z

addon/-private/system/store.js

@@ -2107,7 +2114,11 @@ Store = Service.extend({
    let trueModelName = this._classKeyFor(modelName);
    let owner = getOwner(this);

-    return owner._lookupFactory(`model:${trueModelName}`);
+    if (owner.factoryFor) {
+      return owner.factoryFor(`model:${trueModelName}`) && owner.factoryFor(`model:${trueModelName}`).class;


This could be cached too

rwjblue · 2017-01-12T15:39:05Z

addon/-private/system/store.js

+    let mixin;
+
+    if (owner.factoryFor) {
+      mixin = owner.factoryFor(`mixin:${normalizedModelName}`) && owner.factoryFor(`mixin:${normalizedModelName}`).class;


Seems odd to go through the factoryFor twice like this, I'd suggest a refactor to:

let MaybeMixin = owner.factoryFor(`mixin:${normalizedModelName}`); mixin = MaybeMixin && MaybeMixin.class;

rwjblue · 2017-01-12T15:39:37Z

addon/-private/system/store.js

@@ -2107,7 +2114,11 @@ Store = Service.extend({
    let trueModelName = this._classKeyFor(modelName);
    let owner = getOwner(this);

-    return owner._lookupFactory(`model:${trueModelName}`);
+    if (owner.factoryFor) {
+      return owner.factoryFor(`model:${trueModelName}`) && owner.factoryFor(`model:${trueModelName}`).class;


Same comment as above, we shouldn't call factorFor twice in a row like this...

bmac · 2017-01-12T16:24:02Z

addon/-private/system/store.js

@@ -2034,7 +2034,13 @@ Store = Service.extend({
    // container = < 1.11
    let owner = getOwner(this);

-    let mixin = owner._lookupFactory('mixin:' + normalizedModelName);
+    if (owner.factoryFor) {


Looks like a let mixin; got lost in the squash.

bmac · 2017-01-12T16:52:08Z

Thanks @locks.

workmanw · 2017-01-25T02:20:39Z

I was in the process of fixing up ember-data-model-fragments for ember-data-2.12.0-beta. I noticed that the beta channel for ember-data is missing this commit. I'm definitely in no hurry, I just wanted to call out that it's labeled [BUGFIX canary] but needs to be in the beta channel.

rwjblue · 2017-01-25T14:51:47Z

Ahh, good catch @workmanw. It was likely still canary when the PR was proposed, but once we branched beta for 2.12 the commit prefix didn't get updated.

@bmac - Can this be pulled in for 2.12 beta cycle?

bmac · 2017-01-26T20:04:36Z

Will do.

bmac · 2017-01-30T21:47:05Z

This pr has been pulled into the beta cycle and included in the https://github.com/emberjs/data/releases/tag/v2.12.0-beta.2 release.

rwjblue · 2017-01-30T22:04:10Z

Thank you @bmac!

workmanw · 2017-01-30T23:31:11Z

I'm happy to report ember-data-model-fragments' test pass on all channels. Thanks!!

locks force-pushed the factory-for-deprecation branch from 3f10a03 to df13f60 Compare January 10, 2017 17:52

locks changed the title ~~[WIP] Fix _lookupFactory deprecation for Ember canary~~ Fix _lookupFactory deprecation for Ember canary Jan 10, 2017

locks changed the title ~~Fix _lookupFactory deprecation for Ember canary~~ [BUGFIX canary] Fix _lookupFactory deprecation for Ember canary Jan 10, 2017

bmac reviewed Jan 11, 2017

View reviewed changes

addon/-private/utils.js

if (owner.factoryFor) {

return owner.factoryFor

}

}

Copy link

Member

bmac Jan 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this uses?

fivetanley reviewed Jan 11, 2017

View reviewed changes

bmac reviewed Jan 12, 2017

View reviewed changes

pangratz assigned pangratz and unassigned pangratz Jan 12, 2017

pangratz reviewed Jan 12, 2017

View reviewed changes

locks force-pushed the factory-for-deprecation branch from 5aad35a to 2833861 Compare January 12, 2017 15:38

pangratz reviewed Jan 12, 2017

View reviewed changes

rwjblue requested changes Jan 12, 2017

View reviewed changes

locks force-pushed the factory-for-deprecation branch from 2833861 to c5fe04c Compare January 12, 2017 15:46

rwjblue approved these changes Jan 12, 2017

View reviewed changes

locks force-pushed the factory-for-deprecation branch from c5fe04c to 33859b0 Compare January 12, 2017 15:47

bmac approved these changes Jan 12, 2017

View reviewed changes

bmac reviewed Jan 12, 2017

View reviewed changes

Fixes _lookupFactory deprecation for Ember canary

7ef8273

locks force-pushed the factory-for-deprecation branch from 33859b0 to 7ef8273 Compare January 12, 2017 16:25

bmac merged commit 2f10ee9 into master Jan 12, 2017

bmac deleted the factory-for-deprecation branch January 12, 2017 16:52

bwbuchanan mentioned this pull request Jan 24, 2017

Compat with factoryFor (Ember 2.12ish) #4737

Closed

workmanw mentioned this pull request Jan 25, 2017

[WIP] Removes deprecation from upcoming Ember 2.12 release. adopted-ember-addons/ember-data-model-fragments#240

Merged

Turbo87 mentioned this pull request Jul 10, 2017

Update "ember-data" to v2.11.3 miragejs/ember-cli-mirage#1127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX canary] Fix _lookupFactory deprecation for Ember canary #4743

[BUGFIX canary] Fix _lookupFactory deprecation for Ember canary #4743

locks commented Jan 10, 2017

bmac Jan 11, 2017

bmac Jan 11, 2017

fivetanley Jan 11, 2017

locks Jan 11, 2017

bmac Jan 12, 2017

pangratz Jan 12, 2017

pangratz Jan 12, 2017

rwjblue Jan 12, 2017

rwjblue Jan 12, 2017

bmac Jan 12, 2017

locks Jan 12, 2017

bmac commented Jan 12, 2017

workmanw commented Jan 25, 2017

rwjblue commented Jan 25, 2017

bmac commented Jan 26, 2017

bmac commented Jan 30, 2017

rwjblue commented Jan 30, 2017

workmanw commented Jan 30, 2017

[BUGFIX canary] Fix _lookupFactory deprecation for Ember canary #4743

[BUGFIX canary] Fix _lookupFactory deprecation for Ember canary #4743

Conversation

locks commented Jan 10, 2017

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

bmac commented Jan 12, 2017

workmanw commented Jan 25, 2017

rwjblue commented Jan 25, 2017

bmac commented Jan 26, 2017

bmac commented Jan 30, 2017

rwjblue commented Jan 30, 2017

workmanw commented Jan 30, 2017