[BUGFIX release] MODEL_FACTORY_INJECTIONS is now always false. by stefanpenner · Pull Request #15204 · emberjs/ember.js

stefanpenner · 2017-05-03T22:06:48Z

Double extend means this flag no longer means what it once did. To ensure ember-data functions correctly, we should now force it to always be false.

Code affect in ember-data is: https://github.com/emberjs/data/blob/e6cb564bdb33da4b3c15c3c668dc4b1fdafdf190/addon/-debug/index.js#L37-L39

add some tests ensuring the behavior is as expected
remove from ember-cli blueprints: remove MODEL_FACTORY_INJECTIONS ember-cli/ember-cli#7025
add to website's deprecations: DEPRECATION: Ember.MODEL_FACTORY_INJECTIONS website#2895

stefanpenner · 2017-05-03T22:07:53Z

This (if others agree) should likely join w/e release/beta/canary that disables double extend so that ember-data works correctly even if the user has the flag set to true.

rwjblue · 2017-05-03T23:59:58Z

packages/ember-environment/lib/index.js

-// default false
-ENV.MODEL_FACTORY_INJECTIONS = defaultFalse(ENV.MODEL_FACTORY_INJECTIONS);
+// always false
+Object.defineProperty(ENV, 'MODEL_FACTORY_INJECTIONS', {


We should issue a deprecation if MODEL_FACTORY_INJECTIONS in ENV, and update ember-cli to confirm it isn't specified in the blueprint.

Then instead of setting it to false here, we just leave it undefined (which would also be seen as falsey by ED's assertion).

rwjblue · 2017-05-04T00:03:35Z

packages/ember/lib/index.js

-  set(value) { ENV.MODEL_FACTORY_INJECTIONS = !!value;  },
+  get()      { return false; },
+  set(value) {
+    Ember.logger.warn('As of 2.13.x MODEL_FACTORY_INJECTIONS has no power, feel free to remove it from app/app.js');


Should be an assertion, and this whole Object.defineProperty block should be in an if (DEBUG) {.

Assertion sounds good but I don't think we can strip the flag as users who ignore the deprecation and set the flag to true will have a bad time.

@stefanpenner - Setting it to true has literally no effect in production builds. The only usage that I can see in Ember Data is here which is also in an if (DEBUG) and therefore stripped in production builds.

The only thing we need to make sure to do here, is blow up if someone attempts to Ember.ENV.MODEL_FACTORY_INJECTIONS = <anything> in debug builds, because in production builds there is no behavior changes anyways...

rwjblue · 2017-05-04T00:04:56Z

Also, I agree, we should backport to release branch for a 2.13.1 release.

stefanpenner · 2017-05-04T04:16:38Z

and update ember-cli to confirm it isn't specified in the blueprint.

This is a tad tricky, as it may be confusing for users upgrading ember + ember-cli but not ember-data ? Thoughts?

rwjblue · 2017-05-04T10:58:04Z

The ember-cli blueprints assume the package versions specified in that blueprints package.json. If the user isn't updating ember-data, they can't remove the flag either.

The main things I'm concerned with (and my comments would address) are:

We have finally removed special meaning of this flag in Ember, we shouldn't bloat Embers file size permenantly just to set a flag to a falsey value (which is what it's value would be anyways 😜).
Keeping the flag around, when it has no effect is pretty confusing. A properly worded deprecation (with a link to the deprecations page for more details) can provide the right context to provide folks with guidance.

stefanpenner · 2017-05-04T16:22:50Z

Keeping the flag around, when it has no effect is pretty confusing.

Ya, it has an affect that effect is backward compact to ease upgrading. Let keep it around for an LTS and then drop it? (like other deprecations)

Sounds ok?

rwjblue · 2017-05-04T16:36:34Z

Ya, it has an affect that effect is backward compact to ease upgrading. Let keep it around for an LTS and then drop it? (like other deprecations)

It actually has no effect in production versions (as I mentioned and linked to code for in another comment), I'm super happy to keep both the deprecation (when MODEL_FACTORY_INJECTIONS in ENV) in packages/ember-environment/lib/index.js and either a deprecation or assertion in packages/ember/lib/index.js until 2.16 but both should be in an if (DEBUG) { flag so it is stripped in production.

rwjblue · 2017-05-04T16:37:33Z

FWIW, I checked ember-data back to v2.2.0 and the only thing that it was using the MODEL_FACTORY_INJECTIONS flag for in addon/ dir was this same assertion.

stefanpenner · 2017-05-04T18:35:12Z

It actually has no effect in production versions (

Ya good point. Ok we can strip that one out in prod builds!

stefanpenner · 2017-05-04T19:33:41Z

packages/ember-environment/lib/index.js

+      false,
+      {
+        id: 'ember-metal.model_factory_injections',
+        until: '3.0.0',


@rwjblue what should until be?

@rwjblue should I make this page or is this deprecation self-explanatory?

Hmm, good question. This isn't really a deprecation per-se, just a handy warning that a setting that used to matter doesn't any more.

We can either swap this with a warn (instead of deprecate) or leave it as a deprecation and say until: "2.17.0"...

stefanpenner · 2017-05-04T19:34:22Z

packages/ember/lib/index.js

+        false,
+        {
+          id: 'ember-metal.model_factory_injections',
+          until: '3.0.0',


same as above

stefanpenner · 2017-05-04T19:34:26Z

packages/ember/lib/index.js

+        {
+          id: 'ember-metal.model_factory_injections',
+          until: '3.0.0',
+          url: 'http://emberjs.com/deprecations/v2.x#toc_code-ember-model-factory-injections'


same as above

rwjblue

This is looking good, I'm happy with either deprecate or warn here. If we leave it as deprecate we need to add a section to the website to link to...

rwjblue · 2017-05-04T20:30:49Z

packages/ember-environment/lib/index.js

+if (DEBUG) {
+  if ('MODEL_FACTORY_INJECTIONS' in ENV) {
+    deprecate(
+      'Ember.MODEL_FACTORY_INJECTIONS is no longer required',


I think this should be EmberENV.MODEL_FACTORY_INJECTIONS?

rwjblue · 2017-05-04T20:31:59Z

packages/ember-environment/lib/index.js

+      false,
+      {
+        id: 'ember-metal.model_factory_injections',
+        until: '3.0.0',


Hmm, good question. This isn't really a deprecation per-se, just a handy warning that a setting that used to matter doesn't any more.

We can either swap this with a warn (instead of deprecate) or leave it as a deprecation and say until: "2.17.0"...

stefanpenner · 2017-05-04T20:37:29Z

PR to removing these from blueprints: ember-cli/ember-cli#7025

stefanpenner · 2017-05-05T17:25:55Z

deprecations PR to website: emberjs/website#2895

stefanpenner · 2017-05-06T22:23:53Z

@rwjblue r?

stefanpenner · 2017-05-09T17:01:16Z

@rwjblue r?

stefanpenner · 2017-05-09T22:39:39Z

@rwjblue let me know if this should be [BUGFIX release] or [BUGFIX beta]

stefanpenner · 2017-05-09T22:47:15Z

updated to release as per @rwjblue's suggestion.

mixonic

This seems great. 👍

Double extend means this flag no longer means what it once did. To ensure ember-data functions correctly, we should now force it to always be false.

stefanpenner mentioned this pull request May 3, 2017

MODEL_FACTORY_INJECTIONS + no double extend + polymorphic may have issues warp-drive-data/warp-drive#4966

Closed

stefanpenner requested review from mixonic and rwjblue May 3, 2017 22:11

rwjblue requested changes May 4, 2017

View reviewed changes

stefanpenner force-pushed the model-factory-injections branch 2 times, most recently from 59ad983 to 5568817 Compare May 4, 2017 19:33

stefanpenner commented May 4, 2017

View reviewed changes

rwjblue reviewed May 4, 2017

View reviewed changes

stefanpenner force-pushed the model-factory-injections branch 2 times, most recently from 9c18c16 to 277b009 Compare May 4, 2017 22:18

stefanpenner mentioned this pull request May 5, 2017

DEPRECATION: Ember.MODEL_FACTORY_INJECTIONS emberjs/website#2895

Merged

1 task

stefanpenner force-pushed the model-factory-injections branch from 277b009 to f71c82d Compare May 5, 2017 18:00

stefanpenner force-pushed the model-factory-injections branch from f71c82d to eb99506 Compare May 9, 2017 22:47

stefanpenner changed the title ~~[BUGFIX] MODEL_FACTORY_INJECTIONS is now always false.~~ [BUGFIX release] MODEL_FACTORY_INJECTIONS is now always false. May 10, 2017

mixonic approved these changes May 12, 2017

View reviewed changes

[BUGFIX release] MODEL_FACTORY_INJECTIONS is now always false.

84ba295

Double extend means this flag no longer means what it once did. To ensure ember-data functions correctly, we should now force it to always be false.

rwjblue force-pushed the model-factory-injections branch from eb99506 to 84ba295 Compare May 15, 2017 18:15

rwjblue approved these changes May 15, 2017

View reviewed changes

rwjblue merged commit 73aef49 into master May 15, 2017

rwjblue deleted the model-factory-injections branch May 15, 2017 21:08

ultimatemonty mentioned this pull request Sep 4, 2017

Maintenance/ember 2.15 frolfr/frolfr-web#5

Merged

Uh oh!

Conversation

stefanpenner commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanpenner commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue commented May 4, 2017

Uh oh!

stefanpenner commented May 4, 2017

Uh oh!

rwjblue commented May 4, 2017

Uh oh!

stefanpenner commented May 4, 2017

Uh oh!

rwjblue commented May 4, 2017

Uh oh!

rwjblue commented May 4, 2017

Uh oh!

stefanpenner commented May 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefanpenner commented May 4, 2017

Uh oh!

stefanpenner commented May 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stefanpenner commented May 6, 2017

Uh oh!

stefanpenner commented May 9, 2017

Uh oh!

stefanpenner commented May 9, 2017

Uh oh!

stefanpenner commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mixonic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stefanpenner commented May 3, 2017 •

edited

Loading

stefanpenner commented May 5, 2017 •

edited

Loading

stefanpenner commented May 9, 2017 •

edited

Loading