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

Registry / Container reform #46

Merged
merged 2 commits into from Jul 23, 2015

Conversation

Projects
None yet
10 participants
@dgeb
Member

dgeb commented Apr 9, 2015

RFC

`isolatedContainer` for unit testing. A mechanism will be developed to specify
which initializers should be engaged in the initialization of this instance.
In this way, we can avoid duplication of registration logic, as is currently
done in a most un-DRY manner in the [isolatedContainer](https://github.com/switchfly/ember-test-helpers/blob/master/lib/ember-test-helpers/isolated-container.js#L56-L79).

This comment has been minimized.

@mixonic

mixonic Apr 9, 2015

Member

For Ember-Data we use DS._setupContainer:

If Ember only needs to register/inject, then I would prefer such a hook since they can be easily layered onto a single registry in an arbitrary order.

However I note that you suggest the name be isolatedApplicationInstance - does this imply you will return a real instance and not simply a container with a configured registry?

This comment has been minimized.

@dgeb

dgeb Apr 9, 2015

Member

Yes. If Container is private and only accessible through an ApplicationInstance, then I believe that's the level at which it should be available for testing.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Apr 9, 2015

With the recent container / registry split we were burned pretty badly with our test helpers. We had created quite a few helpers to do proper testing setup/mocking/teardown/etc. I personally am a little weary of the idea of isolating these given that interaction with the container had previously been encouraged for testing and other initializers.

I guess I have no real concrete reason as to this shouldn't be done, other than I don't really want to refactor all my test helpers again.

@tomdale

This comment has been minimized.

Member

tomdale commented Apr 10, 2015

Big, big 👍. I think this proposal kills several 🐦s with one stone:

  1. Simplifies initializers: it was mega confusing knowing whether to call register/inject on the application or the container/registry.
  2. Gives us a public API we can support in a forwards-compatible way, while giving us the freedom to continue to tweak the container and registry implementations.

I personally am a little weary of the idea of isolating these given that interaction with the container had previously been encouraged for testing and other initializers.

I can't speak for everyone, of course, but @wycats and I have been very openly against anyone treating the container API as public. There are cases where you have to use it, but there is a reason it is marked private. This proposal allows us to (finally) provide an API we can encourage people to use.

@workmanw

This comment has been minimized.

Contributor

workmanw commented Apr 10, 2015

@tomdale Certainly my primary use case is around testing. Using containers to initialize and mock stuff. But also, it is one of the arguments provided when creating an initializer. And for a period of time, before services people were encourage to used injection this way. This is one of dozens examples that come up when you Google ember dependency injection. http://ember.zone/ember-application-initializers/

Any ways, just to make my position clear, I'm not against it. Just pointing out it might be a source of pain for some folks.

@tomdale

This comment has been minimized.

Member

tomdale commented Apr 10, 2015

@workmanw Point taken. With Dan's RFC, assuming you were indeed sticking to initializers and coloring within the lines, the refactor is extremely mechanical: just update all of your initializers and replace container with applicationInstance.

@igorT

This comment has been minimized.

Member

igorT commented Apr 12, 2015

container.has/registry.has have been useful for me in the past, so might be good to expose those as well. They don't seem particularly hard to maintain.

@dgeb

This comment has been minimized.

Member

dgeb commented Apr 12, 2015

container.has/registry.has have been useful for me in the past, so might be good to expose those as well. They don't seem particularly hard to maintain.

Seems reasonable. Perhaps has could be exposed as hasRegistration.

@dgeb

This comment has been minimized.

Member

dgeb commented Apr 12, 2015

@tomdale Thanks for your support of this proposal.

@workmanw I'm sorry that the churn in this area has already affected you. I initiated much of this churn with the Registry / Container split as part of the SSR refactor. That was a necessary precondition to the Application / ApplicationInstance split that @tomdale and @wycats performed to enable SSR. Only now that their work is nearing completion is it possible to come back to rethink Registry and Container access. I hope the end result of all this work is a clean set of interfaces that shield developers from any complexity introduced by these refactors. And since these interfaces will finally be thoroughly public, we can avoid similar churn and breakage in the future.

# Alternatives
The obvious alternative is to make `Container` and `Registry` fully public

This comment has been minimized.

@stefanpenner

stefanpenner Apr 12, 2015

Member

most of the in-app usages of container are the result of not having:

  1. class level injection rules (not global injection rules, which don't inherit)
  2. factory injections are backwards, instances should get references to factories and not the other way around.

beyond that, there exist dynamic situations where has/lookupFactory should be exposed, but lookup should never be exposed as this should have been the result of an injection (lazy or constructor).

* Are the public methods listed above sufficient or should any others be
exposed?
* What mechanism should be used to engage initializers in unit and

This comment has been minimized.

@stefanpenner

stefanpenner Apr 12, 2015

Member

Unit don't get initializers. By definition this is then no-longer a unit test. Unit level tests can get collaborates provided as constructor Args, but should have no container awareness. My above comment should help remedy most use-cases where users app code violated this.

This comment has been minimized.

@dgeb

dgeb Apr 13, 2015

Member

All of Ember's "unit" tests rely on the concept of a container that is initialized. The isolatedContainer is not truly "isolated", but relies on registrations performed by Ember and even Ember Data. I'm sure we can all agree that the code in isolatedContainer is un-DRY and a temporary hack, so let's solve this problem more elegantly.

As the discussion about integration vs. unit testing components makes clear, we need a broader discussion about dependency injection, what's exposed to tests, and ember-cli's conventions regarding integration and unit tests. This goes beyond the scope of this RFC, but definitely overlaps it.

@stefanpenner I'll ping you soon to try to set up that overdue hangout / discussion.

@drogus

This comment has been minimized.

drogus commented May 1, 2015

I'm not sure if this is something that fits the scope of this RFC, if that's the case just ignore me.

Lately I've been digging into testing story for Ember.js and one thing bothers me about testing an application with services. At the moment there's no easy way to inject mocked services during acceptance testing, or at least there's no way that I know of. Ember.inject.service looks up factory on a container, which will try to resolve it, so even if a user registers a mock service in the tests setup, it won't be picked up.

It would be great if new public API allowed something like this:

var application = startApp();
var mockService = Ember.Object.extend({
  fetchPosts: function() {
    return [{ title: 'a post' }];
  }
});
application.register('service:storage', mockService);

This would make acceptance tests use a mocked service.

There are multiple ways to handle such behaviour, but all that I know of require preparing code for substitution (like creating adapters in ember-data).

@technomage

This comment has been minimized.

technomage commented May 11, 2015

Ideally we will have a way to register a replacement service as described above, using a public API.

In testing comma components this has proven very necessary to isolate issues in the front end vs. the back end code.

@technomage

This comment has been minimized.

technomage commented May 11, 2015

That was comms components.

@igorT

This comment has been minimized.

Member

igorT commented May 16, 2015

@drogus I am also very interested in the acceptance testing story. I think there are at least three blocking issues:

  1. startApp helper in acceptance tests returns an Application and not an ApplicationInstance, so its impossible to overwrite a registration, you can just register in the fallback case.
  2. resolver always has precedence over the registered classes emberjs/ember.js#11174
  3. this RFC doesn't implement unregister and current unregister doesn't work reliably emberjs/ember.js#11173
@drogus

This comment has been minimized.

drogus commented May 16, 2015

@igorT

Yeah, the only workaround I found to overcome this issues is to reopen resolver:

application.registry.resolver.__resolver__.reopen({
  resolve: function(fullName) {
    if(fullName === 'service:storage') {
      return mockService;
    } else {
      return this._super.apply(this, arguments);
    }
  }
});

which is clearly not very elegant and will break with any bigger internal changes.

@dgeb

This comment has been minimized.

Member

dgeb commented Jun 10, 2015

Here's one way to solve the acceptance testing story:

  • The resolver is set only on the Application, not the ApplicationInstance
  • Acceptance tests receive an ApplicationInstance instead of an Application
  • Custom registrations can be made on the ApplicationInstance.

In this way, registrations will be resolved in order by:

  • The application instance's registry
  • The application's resolver
  • The application's registry

In this way, mocks can be registered with the application instance and those registrations will take precedent when resolved.

internally maintained container:
* `lookup`
* `lookupFactory`

This comment has been minimized.

@bf4

bf4 Jun 12, 2015

Great, so code such as in ember-islands that finds App.DocPrinterComponent would change as below?

-var container = App.__container__;
-var componentLookup = container.lookup('component-lookup:main');
-var component = componentLookup.lookupFactory('doc-printer',  container);
+var component = App.lookupFactory('doc-printer');

This comment has been minimized.

@bf4

bf4 Jun 16, 2015

Or would it be like

App.instanceInitializer({

  initialize: function(instance) {
    instance.container.lookupFactory('doc-printer');
  }
});

(h/t al3xnull in slack https://embercommunity.slack.com/archives/needhelp/p1434479693008755 )

This comment has been minimized.

@dgeb

dgeb Jun 17, 2015

Member

Sorry to miss your earlier question. So the lookup and lookupFactory methods would be exposed directly on the instance, in the same way as register and inject:

App.instanceInitializer({

  initialize: function(instance) {
    instance.lookupFactory('doc-printer');
  }
});

(Note: @stefanpenner has concerns about lookupFactory as it's currently implemented. It may be replaced entirely for 2.0)

This comment has been minimized.

@bf4

bf4 Jun 18, 2015

alrighty, thanks

@igorT

This comment has been minimized.

Member

igorT commented Jun 13, 2015

@dgeb that means that there is parallel register on the container that delegates to the resolver if there is one but caches locally if there isn't?

@dgeb

This comment has been minimized.

Member

dgeb commented Jun 13, 2015

@igorT there is no (non-deprecated) register method on the container. register exists only on registries.

There are two registries now: one on the application instance and one on the application. Each has a register method for custom registrations.

When resolving, the instance's registry "falls back" to the application's registry if it can't resolve. If a resolver is associated with either registry, then it will be consulted before the custom registrations in that registry.

The solution here (which I'm about to PR) is to skip setting the resolver on the instance's registry and only set it on the application's registry. This allows custom registrations made through the instance's registry to take precedence over both the resolver and registrations made on the application's registry.

@dgeb dgeb referenced this pull request Jun 13, 2015

Merged

Container / Registry Reform #11440

3 of 3 tasks complete
@igorT

This comment has been minimized.

Member

igorT commented Jun 14, 2015

My bad I misread The resolver is set only on the Application, not the ApplicationInstance as The registry is set only on the Application, not the ApplicationInstance

@dgeb

This comment has been minimized.

Member

dgeb commented Jul 23, 2015

Implemented and merged via emberjs/ember.js#11440

Should this RFC just be closed now?

@mixonic

This comment has been minimized.

Member

mixonic commented Jul 23, 2015

Yes :-)

The filename should be changed to text/0046-registry-reform.md and the RFC PR and Ember Issue should be added at the top of the text.

@dgeb dgeb force-pushed the dgeb:registry-reform branch from 820f3c3 to aa6a090 Jul 23, 2015

@dgeb

This comment has been minimized.

Member

dgeb commented Jul 23, 2015

Thanks @mixonic! It's ready for review now.

wycats added a commit that referenced this pull request Jul 23, 2015

Merge pull request #46 from dgeb/registry-reform
Registry / Container reform

@wycats wycats merged commit 12c0c29 into emberjs:master Jul 23, 2015

twokul pushed a commit to twokul/rfcs that referenced this pull request Jan 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment