Shazwazza / Smidge

A lightweight runtime CSS/JavaScript file minification, combination, compression & management library for ASP.Net Core
MIT License
364 stars 47 forks source link

Smidge needs to throw if duplicate bunlde names are used for different bundle types #19

Open dazinator opened 8 years ago

dazinator commented 8 years ago

Hello! I'm struggling a bit with this currently. For some unknown reason, when I request a bundled file, i'm just seeing an empty file contents in the browser.

So for example, when I request: /sb/ReachGCv3MvcModuleNavMenu.js.v1 - I see this:

image

If I check my log output, it shows a FileResult is being produced, and the "Bundle" action on the controller is being run: (However, my breakpoint is not hit on that method which is confusing me!):

image

I should also state that I am deriving from the SmidgeController with my own controller:

    public class BundlerController : Smidge.Controllers.SmidgeController
    {
        public BundlerController(IApplicationEnvironment env, ISmidgeConfig config, FileSystemHelper fileSystemHelper, IHasher hasher, BundleManager bundleManager, IUrlManager urlManager) : base(env, config, fileSystemHelper, hasher, bundleManager, urlManager)
        {
        }      
    }

.. and configuring my own routing during app startup (rather than calling UseSmidge():

           //Create custom route
            app.UseMvc(routes =>
            {
                routes.MapRoute(
                    "BundlerComposite",
                    "sc/{file}",
                    new { controller = "Bundler", action = "Composite" });

                routes.MapRoute(
                    "BundlerBundle",
                    "sb/{bundle}",
                    new { controller = "Bundler", action = "Bundle" });

            });

Any ideas on what could be happening? I have incremented the cache version, and verified that the bundle file under app_data definitely has contents.

Shazwazza commented 8 years ago

Any chance you can verify if this works/doesn't work if you are not using your own controller/routes?

dazinator commented 8 years ago

Yep so I am having the same issue when I take my controller and routes out of the equation

dazinator commented 8 years ago

Ok so i'm stepping through the code and managed to get a bit further!

The CompositeFileCacheFilterAttribute finds a file in the gzip folder and returns it.. this is why I don't see any break points hit on the controller or anything as its this filter that's handling things.

E:\src\MyWebApplication\App_Data\Bundler\Cache\Default\2\gzip\myfile.s

So i checked the contents of that file and it is indeed empty! But I am not sure why exactly that it's empty at this point in time.

Shazwazza commented 8 years ago

Smidge does some heavy caching of files. Once the composite file is create, it is written to disk and once that happens it is simply served directly from disk for maximum performance. So you'll need to see how/why that file gets written as empty. To test this, you can make sure the file doesn't exist on disk and then breakpoint in the PreProcessManager.ProcessAndCacheFileAsync , the call to that is made from SmidgeHelper.GenerateUrlsAsync, all of which processes the composite files.

dazinator commented 8 years ago

Thanks, stepping through that code now. I have noticed something odd perhaps unrelated.

So I am creating a bundle like so:

            bundleBuilder.Create("ReachGCv3MvcModuleNavMenu", BundleFileType.Js, "~/Reach.GCv3.Mvc.Module.NavMenu/js/modules*");

So that's a directory containing multiple .js files.

Then when i'm stepping thrugh PreProcessManager.ProcessFile it's doing this:

   var hashName = _hasher.Hash(file.FilePath) + extension;

The thing is, the file path already contains the extension (.js) so it's appending another .js) to it.. not sure if that's intended for some reason. Not sure if this is related to my issue yet, but just wanted to raise it just in case.

dazinator commented 8 years ago

Ok so those methods are producing non empty files, under the cache directory App_Data\Bundler\Cache\Default\3 so I can't see anything wrong there.

The problem is that the CompositeFileCacheFilterAttribute doesn't return those files, it returns some file from a gzip directory - and it's that file that is getting created empty. I haven;t yet tracked down the code that creates that gzip folder and empty file but will keep looking!

dazinator commented 8 years ago

Found the method that creates the gzip cache file. it's the SmidgeController CacheCompositeFileAsync method, juts looking at that now to see why its creating an empty file

dazinator commented 8 years ago

Odd, so I stepped through the code and this time the file its created has contents.. I hope this isn't a race condition scenario or something ><

dazinator commented 8 years ago

Oh crikey, i think this might have something to do with the fact I am using the same bundle name for my css and js bundles...

dazinator commented 8 years ago

So this method in the SmidgeController:


 public async Task<FileResult> Bundle(
            [FromServices]BundleModel bundle)
        {  
            var found = _bundleManager.GetFiles(bundle.FileKey, Request);
            if (found == null || !found.Any())
            {
                //TODO: Throw an exception, this will result in an exception anyways
                return null;
            }

When it makes the _bundleManager.GetFiles(bundle.FileKey, Request); call, if you have a css and a js bundle with the same name, it doesn't appear to select the right bundle files. This then means it can pick the wrong processor etc as it picks the processor based on the first file type of the bundle.

dazinator commented 8 years ago

Haha.. yep Smidge doesn't work if you use the same name for js and css bundle. It doesn't throw any obvious exception either that I can tell. I've changed my bundle names and now all is working fine.

Shazwazza commented 8 years ago

Good find! I think we'll need to add an exception in this case to make it obvious

dazinator commented 8 years ago

Yeah.. I think perhaps the Bundles class, wherever it calls _bundles.TryAdd(bundleName, ) - I assume that the TryAdd is actually returning false where that bundle name has already been added, but Smidge doesn;t currently check that return value, and so the bundle files in my case aren't actually added but Smidge carries on as normal. Can probably chuck in some code to handle that result and throw an exception or something unless you have a better idea for dealing with it.

Any ideas on when you can release a new NuGet package containing the latest merged PR as I am currently still dependent upon my Fork and would like to get back to using the NuGet package!

Shazwazza commented 8 years ago

Sorry been bogged down with a bunch of other work! :) I'm just going through a few of my projects now to get some new releases out. If I don't get to this today i'll hopefully get one out tomorrow.

dazinator commented 8 years ago

No probs, no rush just a nicety from my perspective :)