Validate Ember object types in resolver #11261

mitchlloyd · 2015-05-23T21:30:57Z

@rwjblue This is to address #11212.

This commit adds assertions to ensure that some key object types
are validated when they are resolved:

Routes
Components
Views
Services

I did not add a validation for controller classes because the
implementation for detection would need to be a little different (can't
simply use #detect). Also some of Ember's tests play fast (and loose)
with with the types of things they try to pass off as controllers. For
now I don't think this additional validation is worth adding given
imminent demise of Controllers.

rwjblue · 2015-05-23T21:40:51Z

packages/ember-application/lib/utils/validate-type.js

+  let expectedType = VALIDATED_TYPES[parsedName.type];
+
+  Ember.assert(`Expected ${parsedName.fullName} to resolve to an ${expectedType} but instead it was ${resolvedType}.`, function() {
+    return !expectedType || expectedType.detect(resolvedType);


I would prefer that we stamp the objects in question, instead of checking that they are a specific class.

The idea here is that at some point in the not terribly distant future, we do not want components/views to have to be a specific instance (they might be "POJO's" that adhere to our view systems requirements). Ember.Component already adds isComponent to the instances, we could do the same to the factories.

What do you think?

I'm not understanding the object stamping approach yet. It seems like if components can be POJOs then they shouldn't be validated. This kind of validation only makes sense to me for objects that are integrated with Ember through private APIs.

Discussed with @mitchlloyd in slack. I think he has a path forward...

rwjblue · 2015-05-23T21:42:29Z

This will definitely help folks accidentally exporting the wrong thing for sure. Thanks for tackling!

Can you confirm that both the internal default resolver, and the ember-cli modules resolver follow the same path?

mitchlloyd · 2015-05-24T14:28:45Z

This has been updated to use the factory stamping approach we discussed.

Notice that I renamed isViewClass to be isViewFactory. This property was only used in one place in the code base, but there is some risk that end users may have been using the isViewClass property. However, I can't think of any practical reason that people might do that.

rwjblue · 2015-05-24T16:02:50Z

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

+
+  forEach(types, function(type) {
+    let matcher = new RegExp(`to resolve to an Ember.${capitalize(type)}`);
+    throws(function() {


Ember.assert statements are stripped from production builds, so the prod Travis run is failing (since nothing is thrown). We need to use 'expectAssertion' here, and that helper function will handle the prod case for you.

Aha! Now I know why expectAssertion exists :)

This commit adds assertions to ensure that some key object types are validated when they are resolved: * Routes * Components * Views * Services I did not add a validation for controller classes because the implementation for detection would need to be a little different and some of Ember's tests play fast (and loose) with with the types of things they try to pass off as controllers. For now I don't think this additional validation is worth adding given imminent demise of Controllers.

mitchlloyd · 2015-05-24T16:30:03Z

Can you confirm that both the internal default resolver, and the ember-cli modules resolver follow the same path?

Also, I have an Ember CLI app that I tested with to make sure that the resolve method was used by ember-resolver.

…roller Validate Ember object types in resolver

rwjblue · 2015-05-24T21:01:35Z

Thank you!

igorT · 2015-05-25T17:43:31Z

This seems like it would make it harder to have simple Mock classes in testing that do not have to extend from the actual real Service/Component/etc?

rwjblue · 2015-05-25T17:49:36Z

@igorT - There is no requirement to extend from a particular object. We used a brand specifically to allow it to be overridden fairly easily (MockFactory.isServiceFactory = true does not seem either hard or unusual).

rwjblue · 2015-05-25T18:01:59Z

After discussing in detail with @fivetanley and @igorT, this is a breaking change (Ember Data registers the store as a service, and now it results in an assertion) for services. We need to change the service assertion into a deprecation (which still provides some helpful information) and then we can change to an assertion in 2.0.0.

mitchlloyd · 2015-05-25T18:30:13Z

Makes sense. I was worried about services
since as of today any Ember object could be a valid service. I can get to
my computer and make this update in about an hour unless someone else is
tackling this.
On Mon, May 25, 2015 at 2:02 PM Robert Jackson notifications@github.com
wrote:

After discussing in detail with @fivetanley
https://github.com/fivetanley and @igorT https://github.com/igorT,
this is a breaking change (Ember Data registers the store as a service, and
now it results in an assertion) for services. We need to change the service
assertion into a deprecation (which still provides some helpful
information) and then we can change to an assertion in 2.0.0.

—
Reply to this email directly or view it on GitHub
#11261 (comment).

rwjblue · 2015-05-25T18:31:34Z

@mitchlloyd - I'd love the help on it, no one is tackling yet AFAIK.

stefanpenner · 2015-05-26T17:03:48Z

wait, what if i want to export something else? I suppose just duck type it to have the appropriate flag?

This is likely a breaking change, we should deprecate/warn for a cycle first.

raytiley · 2015-05-28T03:00:33Z

This broke my app. I had a chosen-select component but it extended Ember.Select I know this probably isn't a good practice, but it worked fine up until this change.

It was easy enough to is the isComponentFactory flag on the prototype to fix, but would probably be hard to track down if you don't follow the ember commit log as close as I do.

stefanpenner · 2015-05-28T04:18:38Z

@raytiley it's been updated on master to only deprecate

raytiley · 2015-05-28T12:16:07Z

@stefanpenner thanks. I'm locked into beta right now but will give canary a whirl soon

rwjblue · 2015-05-28T12:16:11Z

@raytiley - Ugh, sorry man.

@stefanpenner - Not the component assertion. The only one that was changed to a deprecation was for Service.

I guess we should also make the component assertion into a deprecation for now. The practice that @raytiley mentions is fairly common (I have used it a few times to get a view usable by name in handlebars).

mitchlloyd · 2015-05-28T12:49:08Z

I made an update here: #11300

Since Ember 1.13, Ember expects that services either extend `Ember.Service`, or they must set `isServiceFactory` to `true` at the class level. Since the Cart service already extends `ArrayProxy`, this commit reopens the Cart service class to set `isServiceFactory` to `true`. See emberjs/ember.js#11261

rwjblue reviewed May 23, 2015
View reviewed changes

mitchlloyd force-pushed the resolver-assertions-no-controller branch from 67394f7 to c299645 Compare May 24, 2015 14:25

rwjblue reviewed May 24, 2015
View reviewed changes

mitchlloyd force-pushed the resolver-assertions-no-controller branch from c299645 to a876722 Compare May 24, 2015 16:25

rwjblue added a commit that referenced this pull request May 24, 2015

Merge pull request #11261 from mitchlloyd/resolver-assertions-no-cont…

d905afa

…roller Validate Ember object types in resolver

rwjblue merged commit d905afa into emberjs:master May 24, 2015

mitchlloyd mentioned this pull request May 25, 2015

Change assertion to deprecation #11273

Merged

mike-north mentioned this pull request May 25, 2015

Make the validations service an Ember.Service DavyJonesLocker/ember-validations#307

Merged

stefanpenner mentioned this pull request May 27, 2015

[1.13.beta-2 Regression] services can no longer extend Ember.Object #11292

Closed

mitchlloyd mentioned this pull request May 28, 2015

Change view and component resolver assertions to deprecations #11300

Merged

rwjblue mentioned this pull request Jun 2, 2015

v1.13.0-beta.2 - Uncaught Error: Assertion Failed: Expected component:chosen-select to resolve to an Ember.Component but instead it was (subclass of Ember.Select). #11324

Closed

This was referenced Jan 19, 2016

Fix isServiceFactory deprecation with Ember 1.13 DockYard/ember-cart#21

Closed

Fix isServiceFactory deprecation with Ember 1.13 DockYard/ember-cart#22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate Ember object types in resolver #11261

Validate Ember object types in resolver #11261

mitchlloyd commented May 23, 2015

rwjblue May 23, 2015

mitchlloyd May 23, 2015

rwjblue May 24, 2015

rwjblue commented May 23, 2015

mitchlloyd commented May 24, 2015

rwjblue May 24, 2015

mitchlloyd May 24, 2015

mitchlloyd commented May 24, 2015

rwjblue commented May 24, 2015

igorT commented May 25, 2015

rwjblue commented May 25, 2015

rwjblue commented May 25, 2015

mitchlloyd commented May 25, 2015

rwjblue commented May 25, 2015

stefanpenner commented May 26, 2015

raytiley commented May 28, 2015

stefanpenner commented May 28, 2015

raytiley commented May 28, 2015

rwjblue commented May 28, 2015

mitchlloyd commented May 28, 2015

Validate Ember object types in resolver #11261

Validate Ember object types in resolver #11261

Conversation

mitchlloyd commented May 23, 2015

rwjblue May 23, 2015

Choose a reason for hiding this comment

mitchlloyd May 23, 2015

Choose a reason for hiding this comment

rwjblue May 24, 2015

Choose a reason for hiding this comment

rwjblue commented May 23, 2015

mitchlloyd commented May 24, 2015

rwjblue May 24, 2015

Choose a reason for hiding this comment

mitchlloyd May 24, 2015

Choose a reason for hiding this comment

mitchlloyd commented May 24, 2015

rwjblue commented May 24, 2015

igorT commented May 25, 2015

rwjblue commented May 25, 2015

rwjblue commented May 25, 2015

mitchlloyd commented May 25, 2015

rwjblue commented May 25, 2015

stefanpenner commented May 26, 2015

raytiley commented May 28, 2015

stefanpenner commented May 28, 2015

raytiley commented May 28, 2015

rwjblue commented May 28, 2015

mitchlloyd commented May 28, 2015