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

Remove isolatedContainer and use Ember.Application.buildRegistry. #49

Merged
merged 4 commits into from
May 18, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented May 14, 2015

The isolatedContainer was way to finicky and caused breakage any time Ember
or Ember Data decided to change what the default contents should be.

This replaces the isolatedContainer with the container built by
Ember.Application.buildRegistry (or .buildContainer) which has been
available since 1.0.0. .buildRegistry does result in a container that is
fully fleshed out but it also has a resolver attached, so in the
non-integration case we replace that resolver with an empty function.

The isolatedContainer was way to finicky and caused breakage any time Ember
or Ember Data decided to change what the default contents should be.

This replaces the `isolatedContainer` with the container built by
`Ember.Application.buildRegistry` (or `.buildContainer`) which has been available
since 1.0.0.  `.buildRegistry` does result in a container that is fully fleshed out
but it also has a resolver attached, so in the non-integration case we replace that
resolver with an empty function.
@rwjblue
Copy link
Member Author

rwjblue commented May 14, 2015

This should fix #46 and possibly #41.

@rwjblue
Copy link
Member Author

rwjblue commented May 14, 2015

FYI - this test suite checks Ember 1.10, 1.11, release (1.12), beta (future 1.13), and canary (future 2.0.0).

@dgeb - I'd like your feedback/review on this one...

@rwjblue
Copy link
Member Author

rwjblue commented May 18, 2015

@dgeb - Ping?

@dgeb
Copy link
Member

dgeb commented May 18, 2015

@rwjblue sorry for the delay - will review tonight or tomorrow morning!

@dgeb
Copy link
Member

dgeb commented May 18, 2015

@rwjblue what do you think of exporting separate methods buildRegistry and buildContainer, where buildContainer takes a registry as an argument?

@rwjblue
Copy link
Member Author

rwjblue commented May 18, 2015

@dgeb - Seems OK to me, what would the purpose/result be? Just to replace the isolatedContainer export?

@dgeb
Copy link
Member

dgeb commented May 18, 2015

@rwjblue I was thinking that it might be useful to be able to build multiple containers from the same registry. But anyway, this is all internal at this point, and my preference is to reform all of this so that tests don't even deal with registries and containers, so I'm 👍 if you just want to merge now and refactor later ...

rwjblue added a commit that referenced this pull request May 18, 2015
Remove isolatedContainer and use Ember.Application.buildRegistry.
@rwjblue rwjblue merged commit f0c8a9a into emberjs:master May 18, 2015
@rwjblue rwjblue deleted the build-registry branch May 18, 2015 17:43
@rwjblue
Copy link
Member Author

rwjblue commented May 18, 2015

@dgeb - Thanks for reviewing!

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