SquishIt icon indicating copy to clipboard operation
SquishIt copied to clipboard

'auto bundling' views

Open AlexCuse opened this issue 10 years ago • 44 comments

See stack overflow post from @syndicatedshannon

http://stackoverflow.com/a/20383699/794

AlexCuse avatar Dec 06 '13 05:12 AlexCuse

I think this is pretty doable, just need to make it a little more flexible/configurable. A way to configure a rendering strategy (so you can just Render() a bundle without any additional info) would be great for general organization purposes and make this way more powerful.

AlexCuse avatar Dec 06 '13 06:12 AlexCuse

It's also a little scary the assumptions I'm making about resource order in my shim. It's bound to cause me a .css debugging nightmare at some point. I'm not sure there's a lot to be done about it, though. If I assume I can include a resource in a control that can itself be included multiple times in the render process and I am still using this auto-bundling, I just have to accept responsibility for writing order-insensitive code (unless each resource specified were to include more complex directives, which starts to defeat the purpose of this auto-bundling).

syndicatedshannon avatar Dec 06 '13 06:12 syndicatedshannon

Would you like me to make a few passes at cleaning it up, or would you like to go right at it and hack away yourself?

syndicatedshannon avatar Dec 06 '13 06:12 syndicatedshannon

Our approach to ordering has always been to output content in the order it was added. Like you said it gets way too complex otherwise. I need to do a bit of research into what you're doing (I clearly didn't understand the virtual path thing, and I need to do some research on the ApplicationInstance.Context stuff you are doing there. It seems like it should work everywhere but not positive.

I gave you commit access to my fork (https://github.com/AlexCuse/SquishIt) so that its easier to keep tabs on each other's progress. I should have some time to spend on it this weekend - if you get there before I do, I propose "AutoBundlingViewPage/Control" for the class names.

AlexCuse avatar Dec 06 '13 13:12 AlexCuse

That odd Context reference you mention is actually not like that in my current project, where I just use HttpContext.Current. Although I don't run unit tests myself, for best practices though I usually avoid using the static HttpContext.Current, and if I recall correctly that little dance is the shortest route to HttpContext.Items from a WebViewPage, where Context is actually HttpContextBase.

It's probably obvious, HttpContext.Items here is just a means to cooperate across all the rendered components. The assumption is that the outermost view has a directive embedded to output all the ResourceLinks that have been collected as the children run, innermost first. This allows children to include links in the HEAD without some of the uglier hacks I've seen.

To the best of my knowledge, there are no natural cases where synchronization is required for HttpContext.Items, even during rendering, as IIS ensures it is allocated exclusively to a Request and components are rendered sequentially at the controller. Maybe you are well-acquainted with all that, but I'm always paranoid about stuffing things in a dictionary when I don't understand how I got it.

syndicatedshannon avatar Dec 07 '13 10:12 syndicatedshannon

Hey Alex, I just tried to drop of a cleaned up version. "remote: Permission to jetheredge/SquishIt.git denied to syndicatedshannon." I'll take a peek tomorrow morning to see if it's something with my account.

syndicatedshannon avatar Dec 07 '13 19:12 syndicatedshannon

I updated my original S/O post with the contents of the file I was unable to commit. It's liberally commented with my thoughts, with the expectation this gives you or someone else the information needed to more easily hack it apart. I'd be happy to keep working on it or hand it over at any point. I'm not sure if a base class is the way to make it most accessible. It may be appropriate to provide the base class as a facade as you've done elsewhere in your project, and perhaps you have an idea of where the methods could sit as extensions.

syndicatedshannon avatar Dec 07 '13 19:12 syndicatedshannon

Sorry I set you up with access to alexcuse/SquishIt.git - I do most of my work in there and try to only push stuff to here once its stable. If you don't commit by the time I get around to it I'll drop it in but would prefer that you get credit for it :)

The context stuff should be fine - I may eventually replace it with the wrapper we use internally (thats pretty easily mockable) if we end up with anything that really needs to be tested.

I'll need to think about whether it makes sense to group by folder by default. I have lots of bundles that combine assets from different folders If we need grouping in discrete bundles maybe it makes sense to have an overload that takes a bundle name or output path relative to a configured base content directory, with the default being View_Path_And_Name.js or something like that

AlexCuse avatar Dec 07 '13 22:12 AlexCuse

ah, my apologies. you did say alexcuse. I'll upload it. I'll derive pleasure from my first github commit. :) I'd appreciate it if you'd attach it to the project though, because I just uninstalled my VS 2010 a few weeks ago.

syndicatedshannon avatar Dec 08 '13 01:12 syndicatedshannon

Is there existing support in SquishIt for relocating img() paths and similar relative changes?

syndicatedshannon avatar Dec 08 '13 02:12 syndicatedshannon

Yes, its the bane of my existence haha. Look at CSSPathRewriter.cs (the test names are pretty informative for that one).

This is probably the best place to look to see what can currently be handled https://github.com/AlexCuse/SquishIt/blob/master/SquishIt.Tests/CSSPathRewriterTests.cs

AlexCuse avatar Dec 08 '13 02:12 AlexCuse

LOL. If it's functional the majority of the time then I'd definitely remove the folder grouping by-default. Either way, I'll go make it a separately-enabled step.

syndicatedshannon avatar Dec 08 '13 02:12 syndicatedshannon

But probably not tonight. :) I've got to finish some real work tonight.

syndicatedshannon avatar Dec 08 '13 02:12 syndicatedshannon

FYI, I'm still working on it. I rearranged it to make the folder isolation optional as we discussed, collapsed the .js and .css bundling methods to use the base methods (which was a challenge for me because it was the first time I've seen T < T > used that way), moved the implementation out to a separate class and added a separate AutoBundlingViewPage and HtmlHelper extension, and was just about to add some de-duplication features.

syndicatedshannon avatar Dec 09 '13 05:12 syndicatedshannon

oh, and also cleaned up an inaccuracy in bundle output order that could occur since breaking out the AddResources into separate AddJsResources and AddCssResources methods.

syndicatedshannon avatar Dec 09 '13 05:12 syndicatedshannon

I recently uninstalled VS2010. It's ugly, because I'm not sure where my media is (which had a physical key with it). You are welcome to wire it up to the project if you like. I didn't want to mess with hand-edits to the .csproj that I couldn't properly verify. (obviously VS2012 and 2013 do all sorts of funky things to the project)

syndicatedshannon avatar Dec 09 '13 08:12 syndicatedshannon

Thank you for integrating that, and my apologies for updating the HtmlHelperExtension references. It looks like that would have broken your build. I switched to using it for production, and found a defect resulting from referencing the wrong enumerable after the refactoring I mentioned above. Still one more refactor to go: to allow de-duplication and better preserve bundle order, I'm going to change when groups are processed to strings relative to when the list gets reversed. This should also allow an option to de-duplicate links, perhaps even with three modes { EmitFirst, EmitLast, EmitAll }.

syndicatedshannon avatar Dec 10 '13 10:12 syndicatedshannon

p.s. Please lay your thoughts on me if you have any.

syndicatedshannon avatar Dec 10 '13 10:12 syndicatedshannon

I forgot, I also mean to add configuration for those public static fields.

syndicatedshannon avatar Dec 10 '13 10:12 syndicatedshannon

Also, since AutoBundler.cs could potentially be used outside of MVC, after it gets locked down, you may want to pick another spot for it.

syndicatedshannon avatar Dec 10 '13 10:12 syndicatedshannon

ugh, still more things I forgot. :) "A way to configure a rendering strategy (so you can just Render() a bundle without any additional info", I'd like to make sure I understand that before this next refactor, since it sounds related. Could you please give me an example?

syndicatedshannon avatar Dec 10 '13 11:12 syndicatedshannon

I haven't fully formed the idea in my head yet but basically want to be able to define the rendering behavior inside the AutoBundler (for example rendering to cache vs filesystem). This strategy could handle filename / key construction too eventually, but I haven't looked closely enough at your code to know what information we'll have available to do so when making the call.

AlexCuse avatar Dec 10 '13 12:12 AlexCuse

I hear you. You mean for example to serve them directly from a controller.

I was considering deferring all rendering until the point when the links are emitted. This would allow some aggregate-aware optimization to be optionally enabled. However I'm a little hesitant to move the render that can result in a file exception error out of the viewpage where that missing source file is specified. The exception generated would show that a file referenced in "MyBaseLayout.chtml" couldn't be found, even though the file was actually referenced in "MyPartialWidget.cshtml". That could get ugly quickly.

What I was thinking may be a suitable compromise was to:

Create a bundle descriptor class. It's a small wrapper around the link, and includes information that uniquely identifies the bundle, its origins, and where it was called from. It's comparable (comparison based upon something highly relevant to your desire for caching) to simplify removing duplicates. It probably understands how to render its contents, and if we really wanted to get complicated could selectively defer Render when it was not in debug mode. The descriptor class also stores a "first seen" and "last seen" index, to add flexibility to de-duplication. The descriptor might store other info such a the bundle size (really reaching here, potentially bundles under a certain size could be inlined in the view).

Decisions of how to render could be made prior to AutoBundler construction (a per-HTTP Request activity), and overridden in a call to AddResources from a ViewPage. When it's time to emit, the emitter spins through the descriptors removing duplicates, ordering as appropriate.

syndicatedshannon avatar Dec 10 '13 13:12 syndicatedshannon

Exactly - there are a lot of rendering options and I'd like to make sure it works reasonably well for all of them. Need to test with custom renderers at some point too (eg https://github.com/AlexCuse/SquishIt.S3)

I'd lean toward keeping it simple for now - the optimization can get hairy, and will probably be easier to pull off if we can get a working foundation in place. If we get clever with naming (eg derive name from all the assets included in a bundle instead of the view it was declared in) I think we may be able to pawn some of it off onto squishit internals also.

I've been getting crushed at work lately but should have time to work on the rendering strategy sometime soon.

AlexCuse avatar Dec 11 '13 13:12 AlexCuse

Sure, that makes sense. I don't remember, but I probably just grabbed VirtualPath as an identifier because it was easy and sufficient for my scenario. At the moment it would break if more than one call to AddResources of a type was made for a single view or partial anyway.

syndicatedshannon avatar Dec 12 '13 04:12 syndicatedshannon

LMAO @ "frankly unsupportable behavior"

                // Note that on a typical MS "preserve-but-ignore-case" posix-compliant filesystem,
                // this case-insenstive grouping does allow for resources in two different folders to be bundled together,
                // however this case would require some frankly unsupportable behavior from the author to induce.

AlexCuse avatar Dec 16 '13 23:12 AlexCuse

I checked in an example of using SquishIt's hasher to build a hopefully unique (and definitely nonsensical) name for a bundle. This should help in a situation with reuse where you have multiple views declaring the same bundle.

I'm trying not to mess with it too much while you are still working on it, but thought I'd show you what I meant.

Do you want to add a view that tests this out to the AspNetMvcTest project? We could set up a new folder/controller for testing autobundling behavior.

AlexCuse avatar Dec 16 '13 23:12 AlexCuse

Awesome, thank you. I will look at it tonight. Sorry to tell you I had it under revision and then no updates for a week. Just busy with the holidays and I spent a little more time on it last weekend than I should have.

syndicatedshannon avatar Dec 17 '13 03:12 syndicatedshannon

Quick question regarding your hash suggestion, do you think it is fair to assume than "a.resource" + "b.resource" == "b.resource" + "a.resource"? I suppose it should be configurable as well...

syndicatedshannon avatar Dec 17 '13 03:12 syndicatedshannon

Regarding:

    //TODO: figure out how to support different invalidation strategies? 

That was the goal of the "_#" here:

    string filename = GetFilenameRepresentingPath(viewPath) + "_#" + bundleExtension;

Does that not do what I thought it did? I did some tests on that when I first wrote it, but really I was designing by braille.

Also, I think we should probably use the full project path of the resources, on the off chance that someone uses some strategy to allow relative paths to resources.

syndicatedshannon avatar Dec 17 '13 04:12 syndicatedshannon