Combine and minify non-external scripts #517

Closed
meteficha opened this Issue Mar 27, 2013 · 11 comments

Comments

Projects
None yet
3 participants
Owner

meteficha commented Mar 27, 2013

widgetToPageContent could be modified to, instead of creating a <script> per included script, combining and minifying the scripts.

There is one big problem, though: if you combine and minify everything, it's very possible that different pages will have different minified scripts, thus leading to reloading a lot of code and defeating the purposes of the optimization. (Another problem is it would be nice to be able to avoid the minification on dev mode.)

To solve this problem I think that we should delegate a bit of power to the Yesod user. They know which scripts are used everywhere and which are used on just a few places. There a few solutions I can think of:

(a) We could provide plural versions of the addScript family of functions. Those scripts within a single call would be combined and minified. The old API would still exist and simply put a <script> tag as it does right now. (I'm using Yesod 1.1 types, please bear with me.)

addScripts :: [Route master] -> GWidget sub master ()
addScriptsAttrs :: [(Route master, [(Text, Text)])] -> GWidget sub master ()

(b) We could provide a barrier function that splits the combination.

scriptCombinationBarrier :: GWidget sub master ()

Scripts above would be combined together. Scripts below, ditto. Using this approach we could even combine Julius files with static ones.

(c) We could provide a higher-order function that combines what goes inside it.

combiningScripts :: GWidget sub master a -> GWidget sub master a

It seems to me that (a) is the simpler one. I included the others for completeness sake, they may spark some idea on someone else's head =).

This feature will allow Yesod to be be used even more efficiently without too much hassle. Right now I need to manually combine and minify my static JS files.

Owner

meteficha commented Mar 27, 2013

Perhaps this issue should be extended for CSS as well.

Owner

gregwebs commented Mar 27, 2013

what I mentioned in the thread that I am doing now: https://gist.github.com/anonymous/4329559

Owner

meteficha commented Mar 27, 2013

Wow, I completely missed this thread for some reason! Thanks for bringing it up!

Owner

meteficha commented Mar 27, 2013

Nice code, you went via (a) that I've rediscovered. A possible optimization would be creating the hash ourselves by concatenating the hashes of the inputs, thus a variant of addStaticContent wouldn't need to look at the contents of the files to calculate the hashes nor the files would have to be loaded if the hash is cached.

Of course, it wouldn't be faster than the onceFoo approach, but at least it would be completely safe.

Owner

snoyberg commented Mar 29, 2013

As promised, here's the approach we have at FP Complete:

https://gist.github.com/snoyberg/5269385

Notice that it all lives in TH land, so all the combining and minification occurs at compile time.

Doing this for JS is relatively straight-forward; the tricky part is getting CSS right due to relative paths. Note the comment in my code.

Owner

meteficha commented Apr 1, 2013

The TH solution looks better to me since it doesn't require the use of the unsafe onceFoos. Also, you seem to have tackled the CSS problem as well =).

Regarding the tricky part you mention, since Lucius is going to parse the files anyway, wouldn't it be possible to traverse it changing the URLs after it's parsed and before it's printed? Then the problem just becomes finding out what how the relative path has changed.

To solve this one problem, all relative paths could be translated to absolute paths. To do so, we'd need to get the path of the original CSS and strip the filename from it. Then a combining function would remove a path piece for each ../ and glue them afterwards. Of course, this would increase the size of the CSS somewhat, but it would be guaranteed to work regardless of where the final CSS will land.

Does this look feasible or madness? =)

Owner

snoyberg commented Apr 2, 2013

We'd need to perform more parsing of the CSS in Lucius than we do currently, to find the url(...) bits. Performing the de-relativization™ is certainly more complicated, but not impossible. I'm not sure if we need to use the Lucius parser for this, however; a simple search for url( might be sufficient.

@snoyberg snoyberg added a commit that referenced this issue Apr 21, 2013

@snoyberg snoyberg Static file combining #517 d01d6fa

@snoyberg snoyberg added a commit to yesodweb/yesod-scaffold that referenced this issue Apr 21, 2013

@snoyberg snoyberg Combine CSS/JS yesodweb/yesod#517 fe2e2a0
Owner

snoyberg commented Apr 21, 2013

OK, I've pushed a commit implementing this, plus added a commit to yesod-scaffold to make use of it. Can you guys have a look?

Owner

gregwebs commented Apr 22, 2013

that works for me, thanks!

snoyberg closed this Apr 22, 2013

Owner

meteficha commented Apr 22, 2013

that-changes-everything

Thanks, Michael, awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment