Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Naming inconsistency between [Activate] and @inject #2151

Closed
justinvp opened this issue Mar 10, 2015 · 16 comments
Closed

Naming inconsistency between [Activate] and @inject #2151

justinvp opened this issue Mar 10, 2015 · 16 comments
Assignees
Milestone

Comments

@justinvp
Copy link

Is there a reason why these are named differently?

  • In Razor it's currently @inject
  • In Mvc it's currently [Activate]

Why not [Inject] and @inject, or [Activate] and @activate?

@eamodio
Copy link

eamodio commented Mar 13, 2015

I wondered the same thing. And inject makes a lot of sense, but activate? Seems very ambiguous what the intent of that is.

@danroth27
Copy link
Member

@rynowak @DamianEdwards @Eilon Do we still plan to move from [Activate] to [FromServices]?

@justinvp
Copy link
Author

Personally, I prefer [Inject] over [Activate] and [FromServices]. I agree with @eamodio on the ambiguity of "activate".

Whatever name is chosen, I really hope it is consistent between Mvc and Razor. Using different names for the same thing increases the concept count unnecessarily (more cognitive load).

I'm not sure [FromServices] will be as nice in Razor... @fromServices?

@eamodio
Copy link

eamodio commented Mar 14, 2015

I will grant you that [FromServices] is better than [Activate], but I would still strongly argue for [Inject] or [Injected]. IMO, using "inject" aligns well with Dependency Injection while also expressing more with less confusion than [FromServices].

Both basically tell me that the property will be set by someone/thing else, but [FromServices] feels oddly specific about who/what will inject it - while at the same time "Services" is so ambiguous that it feels confusing.

While [Inject]/[Injected] seems much less about how the property will be set and more about the fact that it will be.

Just my 2c.

@ctolkien
Copy link

👍 on [Inject]

@danroth27 danroth27 modified the milestones: 6.0.0-beta4, 6.0.0-beta5 Mar 19, 2015
@francisconoriega
Copy link

👍 on [Inject] as well

@atrauzzi
Copy link

atrauzzi commented May 5, 2015

I agree, [Inject] sounds a lot better than [FromServices]. Even though most of the time what you're going to be injecting will be a service, the container shouldn't make assumptions about what it's containing and what their intended uses are.

@davidroth
Copy link

👍 on [Inject] as it is clear from the name whats happening.
[FromServices] sounds very ambiguous (What kind of services, is it only for services?, etc.)

@nil4
Copy link

nil4 commented May 5, 2015

👍 for [Inject] rather than [FromServices], it relates clearly to the DependencyInjection package and namespace.

@dougbu
Copy link
Member

dougbu commented May 5, 2015

FYI [FromServices] is named for how that attribute is used elsewhere in MVC model binding. You can add [FromServices] to a property in your model type -- the same way you can use [FromBody], [FromQuery] and so on. Further properties on the controller are now model bound fairly consistently with how action parameters are model bound.

More generally the names of some of these concepts and attributes haven't completely solidified. Interesting discussion here...

@atrauzzi
Copy link

atrauzzi commented May 5, 2015

I'm not sure what the prevaling philosophy is 'round these parts, but I thought models depending on services is an antipattern.

Moreover, am I understanding correctly in thinking that [FromBody] and [FromQuery] are orthogonal to dependency injection? Between all three, [FromServices] seems like the odd-one-out and shouldn't try to be a part of this naming convention.

@danroth27
Copy link
Member

We are definitely hearing a lot of feedback about the naming and behavior of [Activate], [FromServices] and @inject that we would like to address.

First, some guiding principles:

  • We actually don’t want to provide a general-purpose property-injection mechanism (like [Activate] on ViewComponents, TagHelpers), because there are lots of places we can’t/won’t support it, and it will lead to confusion.
  • There’s a difference between the need to access services and the need to access context (ActionContext, ViewContext, etc). User-code will most frequently need to pull in services the user defines, or some common framework-provided services like DbContext/IUrlHelper/IHtmlHelper. Pulling in context is only really needed when writing a POCO controller or a controller-base-class.
  • [Activate] was created to support resolving context objects, and grew over time to be a general property-injection mechanism. An[Activate]-like mechanism is still needed for resolving context in controllers and ViewComponents – the alternative is constructor injection of context, which complicates the story for DI-integration and makes it harder to program against our base-classes.
  • [FromService] is a model-binding concept, and allows arbitrary plugging of services on parameters, models, properties. We’re ok with how this works, and we’re ok with it being limited to controllers.
  • @inject is working as intended. It’s a necessary mechanism and still needed for writing Razor.

Proposal:

  • @inject will be renamed to @service. [FromServices] will be renamed to [FromService] for naming consistency.
  • @inject will only support real services (no ability to @inject ActionContext).
  • [Activate] will be removed.
  • For each context we want to support, we’ll create a specific attribute as a marker. This allows us to validate usage if users find it confusing. We in-general prefer being explicit to just having magic names, like this:
public class MyController
{
        [ActionContext]
        public ActionContext Context { get; set; }
}

@atrauzzi
Copy link

atrauzzi commented May 6, 2015

It's probably okay to restrict the availability of property & method injection to certain scenarios to help guide people. What doesn't make sense is why the system is being designed to discriminate the various types of things it will inject?

Ideally it should just be something that the container can resolve. Whether it's a "Service", a "Context" or anything else - really that's immaterial to the act of injecting.

Possible counter-proposal:

  • @inject remains the same.
  • @inject supports injecting anything the container can resolve - I think it's unrealistic to try and canonicalize certain types as "Services".
  • [Inject] is added which can be used at any level, but perhaps only during certain operations
  • [Activate], [FromServices], [FromService] removed.
  • Contexts are scope-bound to the container and are injected just like everything else.

Ideally the system should be agnostic so that people aren't cornered and so that the API is easier to intuit just by reading the code.

public class MyController {

     [Inject]
     public ActionContext Context { get; set; }

     public ActionResult createMyStuff([Inject] MyStuffService stuffService) {
     }

}

@evil-shrike
Copy link

I would agree with @atrauzzi. For me as a user it was absolutely unclear why it's needed different approaches for injecting "contexts" and "services".
I'm afraid that emphasizing "services" contradicts the idea of DI a bit. It'd be ok for "Service Locator" but not for Dependency Injection - as "dependency" isn't always a service.

rynowak added a commit that referenced this issue May 21, 2015
This change removes [Activate] from ViewComponents. Accessing context
should be done through [ViewComponentContext]. Accessing services should
be done though constructor injection.
rynowak added a commit that referenced this issue May 21, 2015
This change removes [Activate] from ViewComponents. Accessing context
should be done through [ViewComponentContext]. Accessing services should
be done though constructor injection.
@glen-84
Copy link

glen-84 commented May 21, 2015

@danroth27,

Are the context objects taken from the services container, or from somewhere else?

If it's the former, then I agree with @atrauzzi, that a single [Inject] attribute is much more clear.

If it's the latter, then I think that a single attribute would be better. [ActionContext] doesn't tell me what it's doing, and even looks repetitive.

I also prefer @inject to @service, for the same reasons mentioned by @atrauzzi and @evil-shrike.

Finally, it seems to me that [FromService] is inconsistent with the other [From*] attributes, in that all the others bind data from the request, whereas [FromService] simply resolves the object via DI. i.e. The others (I assume) create an instance of the object and bind the properties to data from the request, while [FromService] requests an instance from DI – two different concepts in my mind. For this reason I think that [FromService] could be removed in favour of [Inject].

rynowak added a commit that referenced this issue May 21, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
rynowak added a commit that referenced this issue May 21, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
rynowak added a commit that referenced this issue May 21, 2015
This change completely removes [Activate]. In a controller, you should
constructor injection or [FromServices] to access services.

To access context items (ActionContext, ActionBindingContext, root
ViewDataDictionary) you should use the respected attribute class.

We'd like to consider streamlining this further in the future by getting
down to a single injectable context for controllers, but for now this will
have to do.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] from ViewComponents. Accessing context
should be done through [ViewComponentContext]. Accessing services should
be done though constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] from ViewComponents. Accessing context
should be done through [ViewComponentContext]. Accessing services should
be done though constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change completely removes [Activate]. In a controller, you should
constructor injection or [FromServices] to access services.

To access context items (ActionContext, ActionBindingContext, root
ViewDataDictionary) you should use the respected attribute class.

We'd like to consider streamlining this further in the future by getting
down to a single injectable context for controllers, but for now this will
have to do.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] from ViewComponents. Accessing context
should be done through [ViewComponentContext]. Accessing services should
be done though constructor injection.
rynowak added a commit that referenced this issue May 22, 2015
This change removes [Activate] support from TagHelpers. TagHelpers which
need access to context should use [ViewContext] to have it injected. To
access services, use constructor injection.
@rynowak
Copy link
Member

rynowak commented May 22, 2015

Closing this because we've done some parts of the plan discussed above and that's our resting place for beta-5. By no means do I want to stop the discussion here or avoid discussion of the changes we did decide to make.

See a general announcement here: aspnet/Announcements#28

TLDR summary:

  • @inject is staying as is, we're happy with it and the community feedback is positive
  • [Activate] is gone - we'd rather plug in a 3rd party DI for users who want property injection. There's a consistency issue with MVC providing this and other parts of Asp.Net which don't.
  • [FromServices] is staying as-is for beta-5. The product team is in agreement with some of the criticisms in this thread, we'll be thinking about it more.

See commits:
92dbd89
b393191
af5322e
144c1d3

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests