feat(tree-differ): refactor + allow reuse of Funnel's "includeFile" method#1986
feat(tree-differ): refactor + allow reuse of Funnel's "includeFile" method#1986caitp wants to merge 1 commit intoangular:masterfrom
Conversation
|
@IgorMinar what do you think? I noticed that one of the things slowing the Funnel port down dramatically, was that the TreeDiffer didn't take Funnel's special filtering procedures into consideration. So this refactoring enables it to do that. There's a memory cost, but it does speed things up a bit (although I'm not sure it's faster than the non-incremental funnel). |
tools/broccoli/tree-differ.ts
Outdated
There was a problem hiding this comment.
includeFile as a function is a bit misleading since we already have includeExtensions as an array of strings. Maybe call it includeFilter and IncludeFilterCallback or IncludeFilterFn.
There was a problem hiding this comment.
I'm thinking filePathFilter or maybe just filter for brevity?
|
otherwise this looks great. thanks |
caitp
force-pushed
the
tree-differ-refactor
branch
from
4bb5d61 to
65d9cde
Compare
tools/broccoli/tree-differ.spec.ts
Outdated
There was a problem hiding this comment.
"ignoreFiles callback returns false" => "includeFilter callback returns false"
|
this still looks like a useful change. do we want it despite the funnel challenges? are there other plugins that could benefit from this? |
I'm not sure what else would benefit from it off the top of my head, but it seems likely that others could. I think it's good to land with the nit addressed |
Some refactoring to make the TreeDiffer api a bit more sane. Additionally, in order to be able to reuse the Funnel plugin's more complicated file selection options, without having to compute diffs for many, many unnecessary files, it is now possible to provide a callback to determine whether a file is to be processed or not. This filter is run after filename filters, if any, for extra convenience.
caitp
force-pushed
the
tree-differ-refactor
branch
from
65d9cde to
9ec60b4
Compare
There was a problem hiding this comment.
I'd like to get #2053 in with some version of the PR I sent you to avoid caching absolute paths here (if you think it's worth caching at all, it doesn't seem unreasonable). This way, there should be roughly one set of cache map transitions on average, and new ones won't need to be created later on
mhevery
added
area: build & ci
|
I'm extracting the code from this PR and redoing this PR. I'll keep it open in the meantime. |
naomiblack
added
action: cleanup
|
this is being reworked in #2257 |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
5 participants
Some refactoring to make the TreeDiffer api a bit more sane.
Additionally, in order to be able to reuse the Funnel plugin's more complicated
file selection options, without having to compute diffs for many, many unnecessary files,
it is now possible to provide a callback to determine whether a file is to be processed
or not. This filter is run after filename filters, if any, for extra convenience.
For performance, the "shouldBeProcessed" status of a file is cached and need not be computed
more than once.