davidebbo / WebActivator

Apache License 2.0
242 stars 49 forks source link

Config abstract class introduced #4

Closed sobanieca closed 11 years ago

sobanieca commented 11 years ago

Hello,

I'm sending a pull request. I have made quite big changes so they need community review. My proposition is not to force developers to use attributes, but instead to force them to define classes in App_Start folder that inherit from abstract class Config. This will allow to achieve greater fliexibility, because one will be able not only to perform single startup code for an application but also to attach event handlers (just like an HttpModule does). For this reason abstract class Config contains a following methods:

PreSetup() - executed before Application_Start() (once per App domain) Setup() - executed after Application_Start() (once per App domain) AttachEventHandlers(HttpApplication context) - executed once per HttpApplication instance. Method for attaching events like BeginRequest, EndRequest etc. Shutdown() - executed at the end of application (once per App domain)

All these methods are virtual, so there is no need to implement all of them. One can just override the method that he wants to use (I suppose that most NuGet packages will override Setup() method). I believe that abstract classes are more object oriented and allow to make unit testing a bit easier - one can just create a test for a single startup task.

Summarizing, in this fork, if someone wants to add a startup task he should create a class that inherits from Config class, say MyStartupTaskConfig and place it inside App_Start folder. This will be quite consistent with current convention that has been introduced in ASP.NET MVC 4.

I have also added virtual property to Config class - Priority. It is enum with following values: {VeryLow, Low, Normal, High, VeryHigh}. I believe that enum values will be better choice than Int32 values for startup config tasks order. The reason for this is because WebActivator is being used by many independent libraries and it will be easier for developers to specify a priority letting them now that their task may run in parallel with other tasks with same priority from different libraries.

Of course everything is backward compatible - attributes are still there. Unit tests and sample web app have proved that nothing has been "destroyed".

I hope that you will accept this pull request and SIGN the assembly (unfortunately applications that I'm developing are signed) so I will be able to use it in my own NuGet package :)

Regards, Adam Sobaniec

davidebbo commented 11 years ago

One major concern here is the perf and memory implication of having to look at every type in every reference assembly to find those that extend your base. With a large app with many large assemblies, this can make a measurable difference.

In fact, that's the very reason why WebActivator was written to rely on assembly level attributes. They're a little quirky to use, but they are very cheap to check for (via Assembly.GetCustomAttributes).

Stepping back, what are the specific scenarios that you think are not covered well today by WebActivator? Is the use of assembly level attributes that painful? :)

sobanieca commented 11 years ago

Not at all :) I just wanted to create a package for logger that I've written and I needed to attach event handlers for HttpApplication like EndRequest and Error. I think that with such base class the similar process would be simpler. I know that process of finding all classes is a bit of performance hit, but since it is being run only once, I thought that it may work like that.

Anyway - it's just food for thought. I can leave with assembly level attributes. What stops me from using WebActivator is the lack of strong name. I see that there is a bit of discussion regarding it - I don't know what is the best solution in this case, but I hope to see something like WebActivator.Signed on NuGet soon ;)

davidebbo commented 11 years ago

Yep, the signing is being discussed in #3.

Note that registering an IHttpModule is pretty easy. Just call HttpApplication.RegisterModule() from your PreStart method. That only exists in 4.5, but in 4.0 you can do the same via Microsoft.Web.Infrastructure.DynamicModuleHelper.DynamicModuleUtility.RegisterModule(). Then you can do whatever you want from the module's Init. Sure, it's slightly more work than your AttachEventHandlers(), but not so bad.