-
Notifications
You must be signed in to change notification settings - Fork 9
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 vs Resolver priority #43
Comments
Good catch. We should discuss. The code and tests match the behavior in Ember, but I find it easier to rationalize overrides that can be manually registered in the registry. /cc @krisselden @tomdale |
Yes, I was hoping the README was the correct one, it makes more sense to me that way too. |
Ember uses the registry for defaults which the user can override by creating a file on disk at the right location. We need something that satisfies that mechanism. You could define a new default resolver for example. We should keep this use case in mind if we make a change. I would be sad to change it based on feels and break the compatibility path with Ember (we're pretty close, I have Ember working with glimmer/di in a branch). Additionally it is worth noting the behavior has already slightly diverged from Ember. In Some context: Registries can have fall-back registries. For example in Ember we use fallback registries so the registrations for Because in
However in Ember, the registry manages the resolver. For an Ember application the check is:
So a direct port of
|
Thanks @mixonic. I would certainly not want to make integration harder. It sounds like we should just update the README to match the existing code. |
If it's possible, I would really love to find a way to make the registry take precedence over the resolver. Generally speaking, in programming the explicit always overrides the implicit. The fact that it's switched here is a stumbling block for many people digging into the Ember DI system, myself included. (Clearly, since I wrote the README and got it wrong. 😄 ) That said, I don't want to make Ember integration onerous. Ember has to do some contortions internally to handle the fallback case, so I think switching this might actually clean up some app initialization code inside the framework. My biggest concern is addon compatibility, which I don't know enough about to feel confident proposing a migration path. Given that the entire question is an order of lookup/fallback priority, is this not something that we can make easily configurable in One option would be to have a "load path" like array of resolvers or registries. Lookup could proceed through the array in order, moving to the next resolver/registry if the previous one did not return a result. Ember could then swap the default order of these. Alternatively, we could establish a protocol for fallback lookups by installing a well-known symbol or property on resolvers/registries. If a resolver/registry didn't return a result, we would use the resolver/registry installed on that property, and so on, until either a result was found or we reached the end of the chain. I don't think we have time to do a big overhaul of Ember's DI system, so I don't want to propose anything that requires big changes to Ember. But I think this is a rare opportunity to revisit precedence order, and it's something that trips up almost everyone because of how unintuitive it is. If we can find a way to accommodate both constraints, that would be awesome. |
@tomdale I have been mulling this over in much the same terms, yet you somehow beat me to posting even while on vacation ;)
This is the approach that I was going to propose. It would allow us to remove custom "fallback" logic from the registry, and allow the container to interact with both resolvers and registries through a common shared interface.
I'm pretty sure that we'd only be adding flexibility at the DI layer, and I think that flexibility could be tuned to keep Ember working "as is". The TBD portions (in my mind) are options registrations and instance vs. factory registrations (see #21). We should decide on a cohesive proposal to minimize churn. Obviously, I'd like to run any proposals by @mixonic and test out any implementations with his unification work as a sanity check. |
The readme says
But I am seeing the oppposite behavior here in the container and here in the tests.
Is this a bug in the code or in the docs?
The text was updated successfully, but these errors were encountered: