[BUGFIX] Change factoryFor to use factoryFor().create() #443

jasonmit · 2017-01-26T04:04:50Z

Discussion: jasonmit/ember-i18n-cp-validations#29

This will extend types, with the owner preserved, while still allowing us to access class properties.

Squash on merge.

jasonmit · 2017-01-26T04:08:24Z

addon/validations/factory.js

@@ -189,6 +189,14 @@ function normalizeOptions(validations = {}, globalOptions = {}) {
  });
 }

+function classProperty(value, property) {


BaseValidator requires this since it's imported and so does not go through factoryFor. This method knows how to deal with reading constructor props off types output from factoryFor and regular types.

jasonmit · 2017-01-26T06:16:48Z

addon/-private/factory-for.js

@@ -2,13 +2,30 @@
  Utility to use `ApplicationInstance#factoryFor` if available (2.12+) and
  fallback to private `_lookupFactory` otherwise
 */
-export default function factoryFor(owner, type) {
-  let factory;
+class LegacyFactoryForWrapper {


Mocks the same public API that we rely on so we can consistently use class/create throughout the rest of the app.

jasonmit · 2017-01-26T06:17:44Z

addon/-private/factory-for.js

  if (owner.factoryFor) {
-    let maybeFactory = owner.factoryFor(type);
-    factory = maybeFactory && maybeFactory.class;


The bug stemmed from this since we end up trying to instantiate object from maybeFactory.class which do not carry forward the owner. What we want is really to return maybeFactory.

I thought that factoryFor was a drop-in replacement. Let me ask someone that understands the new feature.

It is if you're not accessing props off the class, which is only available now via the class prop. Instances should not be created from that class prop AFAIK and which led to the owner not being injected.

offirgolan · 2017-01-26T06:58:23Z

@jasonmit this is a sleek approach but im not sure if its too much? To make the solution and code readability simpler, would it not be easier to just assume the no class returned from the factory carries over the owner and force inject it on create instead via owner.ownerInjection()?

May I'm just missing something here...

@rwjblue have you thought of polyfilling factoryFor to avoid this kind of problem?

jasonmit · 2017-01-26T10:55:29Z

Unsure what you mean by "assume the no class returned from the factory carries over the owner".

jasonmit · 2017-01-26T10:57:54Z

Any attempt to return the owner from the class would introduce an unnecessary extend which would defeat the new factoryFor API.

cibernox · 2017-01-26T11:00:07Z

I think that we might not understand the new factory for. I've asked for help in slack and linked the original issue. Let's see what experts say is the right way of having the owner injected. I suspect that this might be because of the reopen. Perhaps it should be a reopenClass

jasonmit · 2017-01-26T18:00:15Z

Should then, while we wait, revert the factoryFor PR as there is problems with anyone relying on the owner (i.e., inject.service) into validators.

offirgolan · 2017-01-26T18:52:09Z

Unsure what you mean by "assume the no class returned from the factory carries over the owner".

@jasonmit what I meant is that we just force inject the owner on create no matter what the situation is since we have access to the owner regardless. That should simplify the problem for now until we get a better alternative.

factoryFor('validator:presence').create(owner.ownerInjection(), options);

Same thing for the Messages class. Thoughts?

cibernox · 2017-01-26T19:04:51Z

For the record, @rwjblue explained me the factoryFor thing in jamesarosen/ember-i18n#431
Apparently it returns some sort of proxy that has a create method that already setups injections, so no need of returning the class.

Revert the change if you want, but I'm going to work on this in a moment. I want to create a failing test case first.

offirgolan · 2017-01-26T19:11:34Z

Im not at my computer right now so I will look into this more in a few hours. Im curious to see if the returned object from factoryFor is a proxy of the class. If so, the solution here is just

return owner.factoryFor ? owner.factoryFor(type) : owner_lookupFactory(type);

Will report back once I have some answers.

jasonmit · 2017-01-26T19:13:54Z

@jasonmit what I meant is that we just force inject the owner on create no matter what the situation is since we have access to the owner regardless. That should simplify the problem for now until we get a better alternative.

Unsure, but I'd say that mimics the behavior of the old API versus embracing the new API which could lead to other unknowns.

Im not at my computer right now so I will look into this more in a few hours. Im curious to see if the returned object from factoryFor is a proxy of the class. If so, the solution here is just

This will not work as the proxy throws (by design) when accessing anything other than class and create: https://github.com/emberjs/rfcs/blob/master/text/0150-factory-for.md#detailed-design (See first bullet)

The implementation asserting gets/sets:
https://github.com/emberjs/ember.js/blob/c05d241fee2b24de83d67da86bdbc5c8177d4266/packages/container/lib/container.js#L185

rwjblue · 2017-01-26T19:33:43Z

FWIW, I am working on a polyfill now to provide the new API to older Ember versions. Hope to have it published later today...

rwjblue · 2017-01-26T23:16:28Z

https://github.com/rwjblue/ember-factory-for-polyfill should do the trick...

jasonmit · 2017-01-27T00:10:23Z

PR updated with @rwjblue's polyfill. If everyone is in agreement on it I'll write some tests.

cibernox · 2017-01-27T00:12:00Z

addon/validations/factory.js

@@ -564,7 +563,7 @@ function getCPDependentKeysFor(attribute, model, validations) {
  let dependentKeys = validations.map((validation) => {
    let { options } = validation;
    let type = validation._type;
-    let Validator = type === 'function' ? BaseValidator : lookupValidator(owner, type);
+    let Validator = type === 'function' ? BaseValidator : lookupValidator(owner, type).class;


Do you need class here? I think I don't fully grasp when the proxied object is enough and when it isn't.

See: #443 (comment)

In this line, cp-validations is accessing constructor props, and has nothing to do with instantiating validators, which cannot be done on anything other than the constructor which lives off class and not the factory manager. This is by the spec as mentioned in the comment above.

I understand. In this case you are not getting the factory to create an instance of that class, you want to read some information from the class itself.

rwjblue · 2017-01-27T00:12:44Z

I'll keep working on this tonight, but I suspect that the polyfill doesn't support Ember < 2.3 yet (though I haven't tested).

jasonmit · 2017-01-27T00:23:59Z

@rwjblue yup, the polyfill looks to fail with <= 2.0.

jasonmit · 2017-01-27T02:06:35Z

addon/validators/base.js

    }

    // If for some reason, we can't find the messages object (i.e. unit tests), use default
-    errorMessages = errorMessages || Messages;
+    errorMessages = errorMessages && errorMessages.class ? errorMessages : Messages;


Once my PR into the polyfill is versioned, this line can be reverted.

Published as v1.0.2.

cibernox · 2017-01-27T21:59:59Z

FWIW LGTM

offirgolan · 2017-01-27T22:30:39Z

Build is failing for Ember <= 2.0. Looks like the polyfill isnt being injected at all.

cc @rwjblue

rwjblue · 2017-01-28T03:47:36Z

ZOMG, should be working now. ember-polyfills/ember-getowner-polyfill#18 + ember-polyfills/ember-factory-for-polyfill@v1.0.2...v1.1.0.

We will need to bump to ember-getowner-polyfill@1.2.0 though.

offirgolan · 2017-01-28T04:03:39Z

WOOT! Awesome job @rwjblue.

@jasonmit can you update the dependencies in this PR? Can we remove ember-factory-for-polyfill or do we need them both?

rwjblue · 2017-01-28T04:05:31Z

Including only getowner polyfill should work, but I'd definitely appreciate testing. The coupling is somewhat complicated, and it's definitely possible that I've mucked something up.

jasonmit · 2017-01-28T06:50:57Z

@offirgolan PR updated. Thanks.

offirgolan · 2017-01-28T08:58:45Z

Released in v3.2.3. Thank you everyone for helping out on this 😸 !

jasonmit mentioned this pull request Jan 26, 2017

Incompatibility with factoryFor changes in ember-cp-validations jasonmit/ember-i18n-cp-validations#29

Closed

jasonmit commented Jan 26, 2017

View reviewed changes

jasonmit mentioned this pull request Jan 26, 2017

Fail test for #443 #445

Closed

cibernox reviewed Jan 27, 2017

View reviewed changes

jasonmit commented Jan 27, 2017

View reviewed changes

Fix for validator missing owner

fb63b24

offirgolan changed the title ~~First pass at changing factoryFor to use factoryFor().create()~~ [BUGFIX] Change factoryFor to use factoryFor().create() Jan 28, 2017

offirgolan merged commit 9703567 into adopted-ember-addons:master Jan 28, 2017

jasonmit deleted the fix-factory-for-api branch January 28, 2017 10:12

rwjblue mentioned this pull request Apr 4, 2017

owner.factoryFor is not a function When access validate() methon in Ember engine #475

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUGFIX] Change factoryFor to use factoryFor().create() #443

[BUGFIX] Change factoryFor to use factoryFor().create() #443

jasonmit commented Jan 26, 2017 •

edited

Loading

jasonmit Jan 26, 2017 •

edited

Loading

jasonmit Jan 26, 2017

jasonmit Jan 26, 2017

cibernox Jan 26, 2017

jasonmit Jan 26, 2017

offirgolan commented Jan 26, 2017

jasonmit commented Jan 26, 2017

jasonmit commented Jan 26, 2017

cibernox commented Jan 26, 2017

jasonmit commented Jan 26, 2017 •

edited

Loading

offirgolan commented Jan 26, 2017

cibernox commented Jan 26, 2017

offirgolan commented Jan 26, 2017

jasonmit commented Jan 26, 2017 •

edited

Loading

rwjblue commented Jan 26, 2017

rwjblue commented Jan 26, 2017

jasonmit commented Jan 27, 2017

cibernox Jan 27, 2017

jasonmit Jan 27, 2017 •

edited

Loading

cibernox Jan 27, 2017

rwjblue commented Jan 27, 2017

jasonmit commented Jan 27, 2017 •

edited

Loading

jasonmit Jan 27, 2017

rwjblue Jan 27, 2017

cibernox commented Jan 27, 2017

offirgolan commented Jan 27, 2017

rwjblue commented Jan 28, 2017

offirgolan commented Jan 28, 2017

rwjblue commented Jan 28, 2017

jasonmit commented Jan 28, 2017 •

edited

Loading

offirgolan commented Jan 28, 2017

[BUGFIX] Change factoryFor to use factoryFor().create() #443

[BUGFIX] Change factoryFor to use factoryFor().create() #443

Conversation

jasonmit commented Jan 26, 2017 • edited Loading

jasonmit Jan 26, 2017 • 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

Choose a reason for hiding this comment

offirgolan commented Jan 26, 2017

jasonmit commented Jan 26, 2017

jasonmit commented Jan 26, 2017

cibernox commented Jan 26, 2017

jasonmit commented Jan 26, 2017 • edited Loading

offirgolan commented Jan 26, 2017

cibernox commented Jan 26, 2017

offirgolan commented Jan 26, 2017

jasonmit commented Jan 26, 2017 • edited Loading

rwjblue commented Jan 26, 2017

rwjblue commented Jan 26, 2017

jasonmit commented Jan 27, 2017

Choose a reason for hiding this comment

jasonmit Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwjblue commented Jan 27, 2017

jasonmit commented Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cibernox commented Jan 27, 2017

offirgolan commented Jan 27, 2017

rwjblue commented Jan 28, 2017

offirgolan commented Jan 28, 2017

rwjblue commented Jan 28, 2017

jasonmit commented Jan 28, 2017 • edited Loading

offirgolan commented Jan 28, 2017

jasonmit commented Jan 26, 2017 •

edited

Loading

jasonmit Jan 26, 2017 •

edited

Loading

jasonmit commented Jan 26, 2017 •

edited

Loading

jasonmit commented Jan 26, 2017 •

edited

Loading

jasonmit Jan 27, 2017 •

edited

Loading

jasonmit commented Jan 27, 2017 •

edited

Loading

jasonmit commented Jan 28, 2017 •

edited

Loading