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

Calling unregister on the registry does not clear out the container #11173

Closed
igorT opened this issue May 15, 2015 · 10 comments · Fixed by emberjs/ember-test-helpers#1216
Closed
Labels

Comments

@igorT
Copy link
Member

igorT commented May 15, 2015

Until the container/registry split, if you were in a test and wanted to stub methods or tap them for a particular class, you could have done the following:

  var MyAdapter = container.lookupFactory('adapter:my-adapter');
  var TappedAdapter = MyAdapter.extend({ ajax..... });
  container.unregister('adapter:my-adapter');
  container.register('adapter:my-adapter', TappedAdapter);

However in the new world, calling unregister on the registry does not work, as we cache the factory when we look it up and calling unregister does not clear out container factoryCache.

cc @dgeb

To be clear I think that having registry know about the containers seems wrong, maybe we could bring back unregister on the container itself?

@stefanpenner
Copy link
Member

this appears to be a regression, as when implemented this did work.

@igorT
Copy link
Member Author

igorT commented Jul 25, 2015

ping @dgeb as resolver refactor got merged

@dgeb
Copy link
Member

dgeb commented Jul 25, 2015

@igorT What ember version are you working with?

Where are you making these calls? In an application or application instance initializer? Are you sure you're working with a container? unregister and register have now (as of 2.0) been removed from the container class and are available only on the registry.

An updated example would be great - an updated failing test would be even better :)

@igorT
Copy link
Member Author

igorT commented Jul 25, 2015

1.13.something last I checked. I'll update my issue/retest with the latest code next week

@dgeb
Copy link
Member

dgeb commented Jul 25, 2015

I was thrown by the example in which you're calling unregister and register on the container, but I understand the core issue now: we need a communication channel between registries and their associated containers so that unregister clears out any associated caches.

@rwjblue
Copy link
Member

rwjblue commented Aug 19, 2015

I am still kinda fuzzy on if this is a bug or not, and if it is what needs to happen.

@igorT - Can you put together a runnable demo (JSBin or something)?

@dgeb - Has this been addressed yet (I know a bunch of work has been done already)?

@dgeb
Copy link
Member

dgeb commented Aug 19, 2015

@rwjblue this hasn't been addressed yet.

There is now some complexity in registry / container relationships:

  • a registry can have multiple containers
  • a registry can have a fallback registry (which is of course recursive)

Because of this, communication up the possible chain of related registries to containers is not straightforward.

For instance, a container might have received a registration from a fallback registry. If that registration is then reset on the fallback registry, would it be expected that a container's entries are cleared? How about if a new registration is added with the same key, but on the container's direct registry (not its fallback registry)? This would then replace the fallback registry's entry, but it's unclear whether previously instantiated objects should be cleared.

The more I look at the problem of registry -> container notifications, the murkier it seems.

The simple alternative is to call container.reset('adapter:my-adapter') after unregister / register. This should force the reset of the entry in the container.

@igorT - although this isn't automatic, will this work for you?

@SeyZ
Copy link

SeyZ commented Dec 3, 2015

@dgeb Thanks for the container.reset tips!

Not sure if it's the same issue but my problem was:

• I register a new DS.Model (with a new adapter and a new serializer).
• Dynamically, I need to add a new DS.attr on this model.
• First, I unregister the old model (with the old adapter and old serializer).
• Then, I register the new model with the new DS.attr in it (with a new adapter and a new serializer).

Without container.reset(...), the method this.store.modelFor('...').eachAttribute(...) does not return my new Attr.

@rwjblue
Copy link
Member

rwjblue commented Dec 3, 2015

Given the new owner concept, and the presence of both a container and registry in the owner I think that we could do the right thing when getOwner(this).unregister('foo:bar') is called. Specifically we can have ContainerProxy implement unregister as:

unregister() {
  this.reset(...arguments);
  this._super(...arguments);
}

As long as RegistryProxy is mixed in before ContainerProxy (which it is here) this means that the owners container and its backing registry are properly in sync. It does not guarantee that other container instances get cleared, but it still seems like a good step forward...

@dgeb
Copy link
Member

dgeb commented Dec 3, 2015

I agree that the new owner concept provides a nice solution that was previously unavailable.

After talking with @rwjblue, I'm going to prep a PR to add an unregister override on ApplicationInstance (instead of ContainerProxy). Registry changes (unregister / register) should only be made on the application instance once lookups start, so I think this will be sufficient (i.e. we shouldn't need special handling for the application's unregister method).

dgeb added a commit to dgeb/ember.js that referenced this issue Dec 3, 2015
`ApplicationInstance#unregister` now overrides 
`RegistryProxy#unregister` in order to clear any cached instances of the
unregistered factory (via `__container__.reset()`).

[Closes emberjs#11173]
rwjblue pushed a commit that referenced this issue Dec 4, 2015
`ApplicationInstance#unregister` now overrides
`RegistryProxy#unregister` in order to clear any cached instances of the
unregistered factory (via `__container__.reset()`).

[Closes #11173]
(cherry picked from commit 8768e67)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants