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

'auto bundling' views #267

Open AlexCuse opened 10 years ago

AlexCuse commented 10 years ago

See stack overflow post from @syndicatedshannon

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

AlexCuse commented 10 years ago

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.

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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?

AlexCuse commented 10 years ago

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.

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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.

AlexCuse commented 10 years ago

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

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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

AlexCuse commented 10 years ago

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

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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

syndicatedshannon commented 10 years ago

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

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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?

AlexCuse commented 10 years ago

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.

syndicatedshannon commented 10 years ago

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.

AlexCuse commented 10 years ago

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.

syndicatedshannon commented 10 years ago

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.

AlexCuse commented 10 years ago

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 commented 10 years ago

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.

syndicatedshannon commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

Also, can I safely feed an illegal character to the hasher rather than that dash in the String.Join?

AlexCuse commented 10 years ago

No worries - been busy here too. I am just trying to minimize my intrusion, since I'll be reformatting files to match the rest of the project (and make it easier for me to read - its amazing how quickly we get locked into a particular style of formatting). If I were to do that now it would make merging hell.

re: "a.resource" + "b.resource" == "b.resource" + "a.resource" no I don't think this is the case. SquishIt combines resources in the order they are added, so if the resources are added in a different order it would represent a new bundle. Whether this is desirable or not is anyone's guess, but there is no way I'm getting into the business of figuring out what order people's files should be combined in :)

re: invalidation. Including the # in the filename (to be replaced by the hash) is just one strategy. In the absence of a hash symbol we append a querystring parameter, and we can also deal with the hash rendered in as a folder name (which gets scrubbed out by IIS rewrite rules). There is a quick breakdown of the different strategies here

Using the full project path could prove challenging, because resolving resources is not done until its time to render. I suspect that the name we're getting now is unique enough, but this is definitely something to consider in case we run into problems.

re: hasher - you should be able to add anything - the hasher just spits out an MD5 hash of whatever string you feed it - as long as you use a UTF-8 character it should be fine, and even going outside that would probably still work.

AlexCuse commented 10 years ago

I should have a good bit of time to work on this over the holidays. Its available now in pre-release form but I'm hoping to have a production-ready release shortly after new years. May have some questions when the time comes to add the test page. Adding a punchlist of what we'll need

can you think of anything else thats absolutely necessary for a first release of this?

AlexCuse commented 10 years ago

Is upgrading the MVC version to 3 going to ruin anything for you? Its installed via nuget and should be easier to work with overall.

AlexCuse commented 10 years ago

What do you think about segregating script and style resources so you don't have to filter on file extension? We'd need to call two add resource methods but this would make supporting less, coffeescript etc... pretty seamless.

AlexCuse commented 10 years ago

Sorry, I started formatting the files the same as the rest of the project. I implemented resource partitioning and preprocessor support (even for calls through generic AddResource method, as long as preprocessors are registered globally) in exchange for the inconvenience.

I see now why that base view was a regular ViewPage before (for non-MVC?) and changed that back. Using the HtmlExtensions in all of the test views now instead, seems to be working great, through there are no partials or anything involved. Still gonna build against MVC 3 though I think - its always a pain to install v2 when I want to work on a new machine.

syndicatedshannon commented 10 years ago

Hey, just reading through your notes. No issues with MVC3, I did work to get back from MVC5 so that I wouldn't be breaking anything in SquishIt. I think we ended up at 2 when I committed without wiring it up, and your Intellisense found the closest suggestion for you.

Re: your re: on cache invalidation. I see what you mean regarding the hash. You agree that the hash is the right indicator, but desire to provide flexibility of where to emit it. That makes sense. I'd like to put something back into the auto-bundler to optionally emit the hashes again, as it provided part of the "auto" in "auto-bundling" :)

I already added public methods for script and style bundles. If you don't want the joint call in your library, that's fine, I'll just put it back in my private collection.

syndicatedshannon commented 10 years ago

I've got some time starting tomorrow, so let me know what your checkout status is, and if I can do anything to help. I forget what I was last working on, will go through my notes tomorrow.

Regarding the origin views. If the information were in the output, I personally would like to see it conditionally added to the standard tags when in debug mode, even if on an invalid attribute. However, if debug output has to fully validate I think the tag is allowed everywhere a Githubissues.

  • Githubissues is a development platform for aggregating issues.