Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issues with normalization in primary (non-fallback) registry. #113

Merged
merged 1 commit into from
Oct 21, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Oct 20, 2015

When Ember internally builds a registry via Ember.Application.buildRegistry it aliases registry.normalizeFullName to resolver.normalize and this becomes the fallback registry. Then Ember creates the primary registry (in ApplicationInstance's constructor) and provides the primary registry's resolver (which is function() { } to allow for registration overrides) with the original (aka from the fallback registry) normalizeFullName.

Unfortunately, when this was initially implemented in ember-test-helpers I missed this nuance and we subsequently had many issues reported for things that the normalize method would have fixed (specifically converting service:fooBar to service:foo-bar before looking up).

It would be better IMO if Ember.Registry could be made aware of this resolver shenanigans, and provide a hook that can be called to "wrapFallbackRegistry" or somesuch so that ember-test-helpers (and ApplicationInstance in Ember) do not need to know about these "magic" methods.

Fixes #108.
Fixes https://github.com/rwjblue/ember-qunit/issues/200
Fixes https://github.com/rwjblue/ember-qunit/issues/199

@rwjblue
Copy link
Member Author

rwjblue commented Oct 20, 2015

For those following along in the other issues, in Ember with the following component:

export default Ember.Component.extend({
  fooBar: Ember.inject.service()
});

When you call this.get('fooBar') the internal lookup that is done is this.container.lookup('service:fooBar') then the resolver's normalize method translates that to service:foo-bar (which is what actually exists on disk in an Ember CLI world). What was happening before this PR, was that a registration of service:foo-bar was made (and is present), but no normalization was done when the object went to lookup service:fooBar so the overriding service was never used.

// but we need to manually recreate them since ApplicationInstance's are not
// exposed externally
registry.normalizeFullName = fallbackRegistry.normalizeFullName;
registry.makeToString = fallbackRegistry.makeToString;
Copy link
Member

Choose a reason for hiding this comment

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

For consistency, we should probably also set registry.describe = fallbackRegistry.describe;

See https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L1347-L1349

Copy link
Member

Choose a reason for hiding this comment

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

Then again, this would not be consistent with the current ApplicationInstance init (although perhaps that should be updated to include describe as well?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@dgeb - Yes, I saw that and decided to match ApplicationInstance's init. This is exactly why I am suggesting a wrapAsFallback method to Ember.Registry that is aware of these things. It seems "wrong" that Ember.Application or ApplicationInstance (and ember-test-helpers) have to do the internal bookkeeping.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm happy to add describe here for now, but I really want to make up an API that can be used by all three parties involved here...

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated to add .describe, please let me know what you think of a .wrap method or somesuch to make this cleaner from ApplicationInstance / ember-test-helpers perspective.

When Ember internally builds a registry via
`Ember.Application.buildRegistry` it aliases
`registry.normalizeFullName` to `resolver.normalize` and this becomes
the fallback registry.  Then Ember creates the primary registry (in
`ApplicationInstance`'s constructor) and provides the primary registry's
resolver (which is `function() { }` to allow for registration overrides)
with the original (aka from the `fallback` registry)
`normalizeFullName`.

Unfortunately, when this was initially implemented in ember-test-helpers
this nuance was missed and we subsequently had many issues reported for
things that the normalize method would have fixed (specifically
converting `service:fooBar` to `service:foo-bar` before looking up).

It would be better IMO if `Ember.Registry` could be made aware of this
resolver shenanigans, and provide a hook that can be called to
"wrapFallbackRegistry" or somesuch so that ember-test-helpers (and
`ApplicationInstance` in Ember) do not need to know about these "magic"
methods.
@rwjblue rwjblue force-pushed the fix-normalization-for-primary-registry branch from 4d7d8a5 to d4f0266 Compare October 21, 2015 14:39
dgeb added a commit that referenced this pull request Oct 21, 2015
…gistry

Fix issues with normalization in primary (non-fallback) registry.
@dgeb dgeb merged commit 165af78 into emberjs:master Oct 21, 2015
@rwjblue rwjblue deleted the fix-normalization-for-primary-registry branch October 21, 2015 15:03
@rwjblue
Copy link
Member Author

rwjblue commented Oct 21, 2015

Released as ember-qunit@0.4.15 and ember-mocha@0.8.5.

@dgeb
Copy link
Member

dgeb commented Oct 21, 2015

@rwjblue - thanks for adding describe. I'm in complete agreement that we need a more elegant solution in ember itself. There's no way this messy logic should need to leak here. Let's try to come up with something after I'm done with the owner change (emberjs/ember.js#11874).

@dgeb
Copy link
Member

dgeb commented Oct 21, 2015

Released as ember-qunit@0.4.15 and ember-mocha@0.8.5.

that was quick! 💯

@rwjblue
Copy link
Member Author

rwjblue commented Oct 21, 2015

@dgeb - Sounds good.

@mydea
Copy link
Contributor

mydea commented Oct 22, 2015

This does not seem to work if a service is injected via an initializer instead of with Ember.inject.service(). See https://github.com/mydea/ember-cli-example-app-for-github/tree/integration-test-service-injection

The test worked while the service was injected directly in the component. When I changed it so the service was injected via an initializer with application.inject('component', 'userSession', 'service:user-session');, the test stopped working again.

@rwjblue
Copy link
Member Author

rwjblue commented Oct 22, 2015

Initializers are not (and should not be) ran in unit or integration tests. They are only ran when booting a full application (which is what is done in acceptance tests). I would suggest using the Ember.inject syntax for this reason.

@mydea
Copy link
Contributor

mydea commented Oct 22, 2015

OK, but this means that there is no way to have an integration test for a component which uses a service which has been injected via an initializer?

@rwjblue
Copy link
Member Author

rwjblue commented Oct 22, 2015

Sure there is, run the initializers function manually in that tests beforeEach, but we definitely suggest that you use Ember.inject.service() instead.

In other words, the "happy path" (using Ember.inject.service) is simple, and the escape value (running the initializer manually) is a little harder/more verbose. This seems like a good tradeoff here...

@mydea
Copy link
Contributor

mydea commented Oct 22, 2015

Great, thanks for clearing this up! Maybe things like this should go in the component test guides?

@Amerr
Copy link

Amerr commented Aug 2, 2016

@rwjblue can you post some examples . how to inject services , which are injected via intializers in ember .

@rwjblue
Copy link
Member Author

rwjblue commented Aug 2, 2016

Basically, do not inject services in initializers if you want to be able to unit/integration test with them easily.

My suggestion above is still the best way forward, to use a service use Ember.inject.service(). See guides information for more details: https://guides.emberjs.com/v2.7.0/applications/services/#toc_accessing-services

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implicit service injection fails in unit tests for dasherized names
4 participants