mcintyre321 / EmbeddedResourceVirtualPathProvider

A custom VirtualPathProvider for IIS - load views and assets from Embedded Resources in referenced assemblies
http://www.adverseconditionals.com/2011/07/portable-aspnet-code-using.html
MIT License
63 stars 42 forks source link

App_Code instead of App_Start #13

Closed djechelon closed 9 years ago

djechelon commented 9 years ago

I have just found that, after downloading package from NuGet and contrary to documentation, the RegisterVirtualPathProvider is created in App_Code rather than App_Start, and Global.asax.cs is not updated to call RegisterVirtualPathProvider.AppInitialize().

I was trying to look in the code where the cs file is created during installation to submit a fix. I'm not a great expert in NuGet, I would be glad to contribute for fixing this :smile:

mcintyre321 commented 9 years ago

I think it's fine as it is:

Typically, a VirtualPathProvider instance is registered in an AppInitialize method defined in the App_Code directory,

https://msdn.microsoft.com/en-us/library/bhesyfec.aspx?f=255&MSPPError=-2147217396

mcintyre321 commented 9 years ago

Apologies if there is a bug, if i'ts that the docs are wrong/incomplete I'll happily accept a PR*.

*actually docs are my fave type of PR ever.

djechelon commented 9 years ago

I guess MS is always right but think about RouteConfig, BundleConfig, etc. are registered in App_Start. And maybe if you didn't stop at the comma you could read , or during the Application_Start event in the Global.asax file.. So those looks practically equal anyway

:laughing: :laughing:

Surely, either code or docs must be updated

mcintyre321 commented 9 years ago

ta v. much!

djechelon commented 9 years ago

Hi, I think we should reopen this topic.

I think it could be an issue to use AppInitialize method. http://stackoverflow.com/a/330720/471213 shows that only one class can expose a public static void AppInitialize() method or then you get a compiler error.

Please forgive for insisting on this topic, but I want to discuss the design choice of using the App_Code strategy for two reason

  1. (Subjective) I personally believe, IMHO, that it is bad design to use "magic methods" as similar as the magic number antipattern (shame on Microsoft first!!). For example I avoid like death using magic events Application_Start and Application_End in Global.asax, instead I use the Init method to wire proper event listeners
  2. (Objective) If a developer uses his own AppInitialize, it would conflict with ERVPP's, thus I believe all open source projects should consider the worst case scenario

Obviously I could, and maybe did already, change the code from App_Code to App_Start in my project, but I care about discussing and sharing thoughts to make better software.

Thank you for your time.

mcintyre321 commented 9 years ago

I'm inclined to agree with you, but I'm trying to remember why I used app_code in the first place.

I suspect that in order to have the package work immediately after INSTALL-PACKAGE it was easier to use App_Code than attempt to modify someone's global.asax using some kind of nuget installer.

I'm not sure if a WebActivator/preapplicationstartmethod can be used or not to achieve the same thing or not, but if so perhaps that would be a better way.

I happy with the project as it stands (as you mentioned, it is easy to work around), but would accept a PR to the effect that:

mycroes commented 8 years ago

I know this topic is (getting) old, but I have to agree with djechelon. I placed the following in a class in App_Start:

using System.Linq;
using System.Reflection;

[assembly: WebActivatorEx.PostApplicationStartMethod(typeof(Shell.EmbeddedResourceProviderStart), "Start")]

namespace Shell
{
    public static class EmbeddedResourceProviderStart
    {
        public static void Start()
        {
            var assemblies = System.Web.Compilation.BuildManager.GetReferencedAssemblies()
                .Cast<Assembly>()
                .Where(a => a.GetName().Name.StartsWith("System") == false);
            var vpp = new EmbeddedResourceVirtualPathProvider.Vpp(assemblies.ToArray())
            {
                //you can do a specific assembly registration too. If you provide the assemly source path, it can read
                //from the source file so you can change the content while the app is running without needing to rebuild
                //{typeof(SomeAssembly.SomeClass).Assembly, @"..\SomeAssembly"} 
            };
            System.Web.Hosting.HostingEnvironment.RegisterVirtualPathProvider(vpp);
        }
    }
}

This also works when placed in the assembly providing the resources, thus making it more of a self-contained solution as well (might want to restrict it to declaring assembly only though in that case). Only thing needed in the startup project to make this work is to setup the web.config handlers section, no direct references required to ERVPP either.

Would you be willing to accept this as a PR? Also with further restriction to declaring assembly?

mcintyre321 commented 8 years ago

I'm still of the opinion that any developer who is using their own AppInitialize is sufficiently capable of fixing a conflict, and also I like that the current approach doesn't require require referencing WebActivatorEx (which I've had versioning issues with before, when referenced packages have required specific versions). On the other hand, App_code compiles weirdly.

@mycroes Your approach makes the following changes right?

  1. + Don't use app_code so it's compiled at build time, not runtime
  2. + Don't use AppInitialize so no conflict
  3. - New reference on WebActivatorEx

I think I'm happy with accepting that.

On the self registration: I don't think it's a good idea to have the assembly providing the resources self register from inside compiled code, too magic IMO, and has scope for double registrations. That said, I'm not sure that the current out-of-the-box approach is too good either though, as it potentially makes every embedded resource in your system callable... Persuade me if you think it's really important!

mycroes commented 8 years ago

Yes those would be the changes. About the registration: I can see two useful approaches:

  1. Startup project references ERVPP and tells which assemblies to expose (current)
  2. Resource hosting projects expose ERVPP with their own resources

Let's assume someone wants to host resources because it's a shared project (I can't really figure any other case why one would want to do this).

The result of option 1 is that you'd have only one VirtualPathProvider for ERVPP that exposes all the (configured) resources in the application. It requires the shared projects to just embed the resources, nothing else. The startup project needs the App_(Code|Start) class to load ERVPP and it might need configuration/customization (I'm not sure what happens when customizing and updating the package, will NuGet prompt or overwrite?) if you don't want to expose all referenced assemblies.

With option 2 the control is moved to the individual projects, and every resource hosting project will need to reference ERVPP and register the VirtualPathProvider. This also means that with n assemblies with resources you'll have n VirtualPathProviders which means that virtual path resolution becomes an O(n) process, contrary to the case where there is only one VirtualPathProvider and the lookup in ERVPP is near O(1) (Dictionary key lookup complexity according to MS).

Something went wrong, I guess I just persuaded myself to go for option 1.