search

broccolijs / broccoli-plugin

Base class for Broccoli plugins
MIT License
25 stars 25 forks source link

Enable plugins to avoid creating cache path. #19

Closed rwjblue closed 7 years ago

rwjblue commented 7 years ago

Many plugins do not use the cache directory, this lets them opt out of its creation. By default, there is no change (cache directory is still created), but if the proper flags are passed to broccoli-plugin when calling super the directory can be avoided.

Making this change and utilizing from broccoli-funnel and broccoli-merge-trees eliminates ~ 231 extraneous directories during initial build of the Ghost admin app.

TODO:

rwjblue commented 7 years ago

Updated to only add a flag to disable creation of cachePath (preventing creation of outputPath only saves a small number of directories from being created, and adds a fairly annoying bit of complexity where the rest of the system can't assume the output paths are present).

rwjblue commented 7 years ago

@stefanpenner / @joliss - I'd love feedback / thoughts here...

joliss commented 7 years ago

I like this, just have to review the code. Let me get some other stuff out of the way first, then I'll get back to this!

joliss commented 7 years ago

Okey-doke! This is looking good.

I reviewed this offline, so instead of leaving comments I made some suggested changes in the code and committed them as [fixup] commits in the joliss/add-capabilities branch, with my comments in the commit messages. Do you want to take a look and squash them into your main commit if you like them?

I also added some commits on top, notably https://github.com/joliss/broccoli-plugin/commit/7ad535927fade536e262ec37a372da104f34ba3a and https://github.com/joliss/broccoli-plugin/commit/fe3c55c7bd584a7c59bf9354aae468305c5897df.

My only remaining question is about the naming. I have a feeling that something like "needsCacheDirectory" ("needCacheDirectory"? "needsCache"?) might fit better than "createCacheDirectory". I think the reason for that feeling is that plugin options (and nodeInfo properties), like "name", "annotation" or "nodeType", are more about plugins describing themselves than telling the builder what to do.

What do you think? If you agree, you could do a global s/createCacheDirectory/needsCacheDirectory/ or similar across the affected repos. If you like "createCacheDirectory" better, or if you don't think it's worth the hassle, then just keep it -- your feature, your call :)

rwjblue commented 7 years ago

Sounds great, thank you for reviewing and working on changes! I've incorporated your changes and tweaked the name (I much prefer needsCache and your reasoning there).

Let me know if you have any more tweaks/updates/suggestions/etc...

joliss commented 7 years ago

I meant to mention btw (not sure how obvious this is): It might look a bit redundant that we're integration-testing things both in broccoli-plugin and in broccoli core. But the reason for that is that each of them tests against a range of older versions of the other. So that way, we get a kind of compatibility matrix that ensures that every version works with every version.

rwjblue commented 7 years ago

It was not immediately obvious when I first started this PR (because I hadn't worked on the Broccoli PR yet), but it became clear fairly quickly.

I really like the capabilities system here...