Feature/caching

[rhys-vdw]

Hey @jdlehman, I just implemented a new feature to cut down build time.

Because I originally didn't include this.cacheable() in the loader, Webpack would rebuild all .erb files on every file change (even if just some unrelated JS file). This means an extra 2-3 seconds before the hot reloader will refresh the page for us.

This PR adds support for the following config options cacheable, dependenciesRoot, and parseComments.

The default behaviour for the loader is to parse each *.erb file for JavaScript block comments starting with uh-erb-loader-dependency. If found, the module will be set to "cacheable", and will only be recompiled if the file itself is changed or if one of its listed dependencies changes.

If not found, the module will use the config bool "cacheable" to determine whether to cache or not. By default this is "false", which means that it will behave the same as the previous versions unless you tell it otherwise.

There's more info and an example in the README

[jdlehman]

Wow this is great! 👏 The only downside to this loader before was the speed of every rebuild on unrelated file changes. The code looks good to me and is readable (well except maybe the regexes 😉 )

I pulled down this branch and everything works as described and provides a much better developer experience with the caching. I think the only thing we might want to tweak is the names of the config options. It is a bit confusing that if cacheable is off, it can still cache if parseComments is true. One of the following may help:

Option 1

Maybe we can condense these two options into a cacheStrategy config option? cacheStrategy=none, cacheStrategy=default, cacheStrategy=dependencies or something like that. Where anything above default will fall back to default if there are no comments found. This might enable other caching strategies in the future, though I can't think of anything we might add now.

Option 2

We could also only have the cacheable key and assume that the user is opting into the dependency caching if they put any comments in their erb files. Downside to this approach is that we always run the regex, but that shouldn't be too expensive (we might have to benchmark this)

Thoughts on this? While I do think it is a bit confusing, it is probably fine to stay as is, if we can't find a good alternative.

[rhys-vdw]

Thanks for the review @jdlehman. :-)

Yeah, I was worried it might be a bit confusing. I think Option 2 appeals to me more. Turning off file parsing might be an bit of a YAGNI. Perhaps it at least doesn't need to be documented.

I took the query parameter 'cacheable' from other loaders I looked at, and I want to follow the same convention. Originally I tried your suggestion and had options true, false and "auto", but that seemed confusing also, particularly because there's no way to say "always cache, regardless of whether there are dependencies".

How about this?

No auto-caching stuff. Instead we have the option cacheable as it is. Even if you add dependencies, files will be cached or not based on the query param. Alternatively you might override per file if you really need to:

/* uh-erb-loader-cacheable false */
/* uh-erb-loader-cacheable true */

Options remain the same, but there is less surprising interplay. Do you think it would make sense to set the default for "cacheable" to true?

[rhys-vdw]

@jdlehman I just went ahead and make those changes. Hopefully it's all a bit more sensible now. I think we should change default cacheable to true.

[jdlehman]

@rhys-vdw these changes look good to me. I think this gets rid of the confusion, and I like the ability to override config options per file if needed.

I also agree about defaulting cacheable to true. :shipit: