Fixes: #14 by upgrading to broccoli-persistent-filter #15
Conversation
|
@stefanpenner, does this look good to you? |
|
@joliss yes it looks good. I will quickly try it out to confirm. (ill look in like 15 min) |
|
just confirmed with a new app (with a few coffee-script files added) that all appears to work, and IO op and cost dropped as expected. I don't have a large app, but i suspect that is fine. If issues arise we can fix them :) |
|
@joliss I have a large app with a lot of coffeescript files, I tested this branch -- it works fine and cuts my coffeescript-specific build time to 15% of what it was on rebuilds. |
|
@joliss please merge and cut a release soon 👍 |
| } | ||
|
|
||
| CoffeeScriptFilter.prototype.extensions = ['coffee', 'litcoffee', 'coffee.md'] | ||
| CoffeeScriptFilter.prototype.targetExtension = 'js' | ||
| CoffeeScriptFilter.prototype.baseDir = function() { | ||
| return __dirname; |
There was a problem hiding this comment.
If I put a throw new Error('x') statement in front of the return, the test suite still passes. It looks like this method never runs. Surely that can't be right?
There was a problem hiding this comment.
When options.persistent = true is provided (it may want to be enabled by default) this code-path is taken.
persistent mode is opt-in, but the tests should stress it.
There was a problem hiding this comment.
Ah. I agree it seems reasonable to make it on-by-default. (It'd seem very strange for the user to enable or disable persistent caching on a per-plugin basis.) Is on-by-default what you do with other plugins?
the tests should stress it.
Yes, definitely, at least a smoke test to exercise the code path.
There was a problem hiding this comment.
Is it options.persistent or options.persist?
There was a problem hiding this comment.
|
@joliss some fast plugins leave it off by default, as they still benefit enormously from the patch based rebuilds. for CS it should very likely be enabled by default |
|
@joliss please help merge this PR. Have been using it for over 2 weeks without issue. |
|
@johnnyshields I can't merge this as it is - see the discussion above. I'm waiting on a new push from @thoov. |
|
@joliss @johnnyshields I have updated the PR. I have also turned persist on by default. |
|
@joliss pinging on this issue, please kindly merge and cut a release if it's satisfactory. |
|
@joliss hola? |
|
@joliss ping |
|
@joliss as per our discussion, i have confirmed (in an although simple app, that add/update/remove/move etc seem to work). I will gladly deal with any bugs/issues that fall-out, I only need commit + release so I can do so without having to bug you. |
Fixes: #14 by upgrading to broccoli-persistent-filter
|
Out in 0.6.0, thanks everyone! :) |
Upgrading broccoli-filter to broccoli-persistent-filter which addresses @stefanpenner performance issue (#14)