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 registry resolver for isolated container @2.3+ #127

Merged
merged 2 commits into from
Dec 7, 2015

Conversation

nickiaconis
Copy link
Contributor

Fixes #126.

@rwjblue
Copy link
Member

rwjblue commented Dec 5, 2015

This does seem like a good fix, but it seems bad that a change in Ember broke like this. I'll try to dig into the change in Ember and see who is at fault (Ember or ember-test-helpers).

@nickiaconis
Copy link
Contributor Author

Ember deprecated registry.resolver being a method (in favor of registry.resolver.resolve being a method), and properly handles cases when the registry is created with a resolvermethod, but only in the constructor. Assignment to the property after the fact is not accounted for. I'm not sure how it would be.

@rwjblue
Copy link
Member

rwjblue commented Dec 5, 2015

Awesome, thank you for doing the research on that!

@dgeb
Copy link
Member

dgeb commented Dec 5, 2015

Assignment to the property after the fact is not accounted for. I'm not sure how it would be.

Yeah, this is the case we decided not to allow and deprecate. Not that we want a deprecation to always happen here - we need a better solution.

@@ -296,6 +296,9 @@ export default Klass.extend({

if (this.registry) {
this.registry.fallback.resolver = function() {};
if (hasEmberVersion(2,3)) {
this.registry.fallback.resolver.resolve = function() {};
}
Copy link
Member

Choose a reason for hiding this comment

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

Might be slightly better as:

      if (hasEmberVersion(2,3)) {
        this.registry.fallback.resolver = {
          resolve: function() {};
        };
      } else {
        this.registry.fallback.resolver = function() {};
      }

Will consider whether this can be handled even more elegantly.

@rwjblue
Copy link
Member

rwjblue commented Dec 5, 2015

@dgeb - I was thinking that we might be able to restructure our usage here, to simply create the registry with the right resolver (instead of clobbering it after instantiation). I believe that would work properly for all versions (and we wouldn't need a conditional), it also makes ember-test-helpers follow the "right" pattern...

@dgeb
Copy link
Member

dgeb commented Dec 5, 2015

I was thinking that we might be able to restructure our usage here, to simply create the registry with the right resolver (instead of clobbering it after instantiation)

Sounds like the right approach to me @rwjblue 👍

@nickiaconis
Copy link
Contributor Author

@rwjblue - I've updated the PR to alter the resolver's resolve method before the registry is created. Is this the way we should be doing it? If this looks good to you, I can squash my commits.

@nickiaconis
Copy link
Contributor Author

It seems the npm install failed on Travis. How can I run it again?
Found it. Closing and re-opening does the trick: travis-ci/travis-ci#576 (comment)

@nickiaconis nickiaconis closed this Dec 7, 2015
@nickiaconis nickiaconis reopened this Dec 7, 2015
rwjblue added a commit that referenced this pull request Dec 7, 2015
Fix registry resolver for isolated container @2.3+
@rwjblue rwjblue merged commit cfbdd69 into emberjs:master Dec 7, 2015
@rwjblue
Copy link
Member

rwjblue commented Dec 7, 2015

Thank you!

@rwjblue
Copy link
Member

rwjblue commented Dec 7, 2015

Released in ember-qunit@0.4.17 and ember-mocha@0.8.8.

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.

3 participants