jetheredge / SquishIt

Lets you *easily* bundle some css and javascript! Check out the Google group if you have questions!
http://groups.google.com/group/squishit
MIT License
459 stars 119 forks source link

Understanding debugStatusReader.IsDebuggingEnabled() #314

Open Worthaboutapig opened 9 years ago

Worthaboutapig commented 9 years ago

I've just started using the debugPredicate (in order to get around that SquishIt doesn't work properly when running async, as there's no HTTPContext in async)

Here: https://github.com/jetheredge/SquishIt/blob/b80deee0f045c8b9b255e25475c4091375808073/SquishIt.Framework/Base/BundleBase.Rendering.Internals.cs line 238:

        if (debugStatusReader.IsDebuggingEnabled()) //default for predicate is null - only want to add to cache if not forced via predicate
        {
            bundleCache.Add(name, content, bundleState.DependentFiles, IsDebuggingEnabled());
        }

I don't understand the comment, it's rather cryptic... only want to do something if not forced to by a predicate which isn't there? Could someone explain this in other words (it's rather hard grokking statements in the negative like this).

Furthermore, it introduces inconsistent behaviour, I'd expect the predicate to be passed-in here, it is explicitly not passed-in? (line 194 further up, in the same method, uses the IsDebuggingEnabled() internal method, which checks the value of the predicate.

The upshot, if this is the intended behaviour, is where we are in debug=true implicitly (where there is a valid HttpContext) then the debugStatusReader.IsDebuggingEnabled() returns true, but if I define a predicate returning true in the configuration, the predicate is ignored in this one place and debugging returns false when I've asked for true. It seems wrong to me- it means it behaves differently in notionally the same circumstance.

As an aside SquishIt doesn't seem to support async calls, is it meant to?

AlexCuse commented 9 years ago

It isn't not meant to, though I don't really see where it would be helpful. I admittedly haven't looked at the async stuff at all. SquishIt needs to calculate a hash based on the combined file's content in order to render a URL into the page, so it kind of needs to operate synchronously. In async scenarios I think you'd want to pre-render into the cache and then retrieve when rendering. I'd welcome a PR, though there is a lot that will probably be tricky without an HTTPContext.

As to your more specific question, you can set up predicates to "force" debug rendering of bundles at runtime. This is useful for determining if CSS or javascript issues are the result of content being combined or not, or for debugging javascript issues in production. These are referenced via the BundleState, and can be configured globally using Bundle.ConfigureDefaults().UseConditionToForceDebugging(..) or on individual bundles using .ForceDebugIf(..). For example I use one that looks for ?SquishItDebug=true in the query string.

The code in question is there because If one of these predicates was used to force debugging and not the status reader, we do not want to include the rendered output in the bundle cache because it is not valid for the application's actual state (running in release mode).

Hope this helps.

Worthaboutapig commented 9 years ago

Thanks, I'll think about the async side, sometimes it works, sometimes is doesn't... though it's probably a matter of how I'm using it ;)

Thanks, I think I get that (though I haven't looked at the status reader and so I don't fully follow your explanation as to why its debug state is different from the debug state specified), but if it's meant to be like that, ok.

I'd suggest the comment could be clearer...?

AlexCuse commented 9 years ago

I will try to think of a better wording. But the fact that my explanation above wasn't enough makes me think I might be incapable :(

Basically we want to ignore the predicate defined on the bundle state when determining whether to cache the bundle. Maybe I will just add another internal method like "ShouldCacheDebugBundle"?

Worthaboutapig commented 9 years ago

:) I'll think about your explanation more deeply so I understand it properly and see if I can suggest a wording as a SquishIt lay-person...

p.s. Much prefer this to the MS bundling :)