Fix getDestFilePath behavior for directories #52
Conversation
|
Thanks for the tests, I will unlikely be able to actually look into this for the next few days. (Traveling for conference, giving talks and such). maybe @rwjblue has some spare cycles, if not I will try to dig in (and answer your question) as soon as I can, but it might be a few days. |
|
Ping @stefanpenner |
elucid
force-pushed
the
dirs-with-extensions-should-not-be-matched
branch
2 times, most recently
from
4e85d48 to
39dccae
Compare
In practice, most directories do not have extensions and as such return null when passed to getDestFilePath in a filter with extensions provided. This is important because canProcessFile by default uses this to determine whether a node should be processed. However, in the case where a directory happens to have a matched extension (e.g. the tree contains loader.js/loader.js and we are doing some processing on js files), we only want the loader.js file, and not the loader.js directory, to be processed.
elucid
force-pushed
the
dirs-with-extensions-should-not-be-matched
branch
from
39dccae to
e73fb44
Compare
|
Hey @stefanpenner, it's been a while, and we still want to make this change :) We've made an attempt at fixing our own failing tests by explicitly testing whether paths are directories to prevent the problem with extensions that broke the excluded from with fingerprint extensions. We've tested this PR by locally npm linking it into both PRs ember-cli/broccoli-asset-rewrite#34 and ember-cli/broccoli-asset-rev#93, and then npm-linking the above broccoli-asset-rev PR into our product app. This takes rebuilds from ~10s to ~600ms. We've also tested with a new ember app, npm linking in the same way. Anyway, eager to get your thoughts on this. /cc @ghedamat |
elucid
changed the title
| @@ -210,7 +210,26 @@ Filter.prototype.canProcessFile = | |||
| return !!this.getDestFilePath(relativePath); | |||
| }; | |||
|
|
|||
| Filter.prototype.isDirectory = function(relativePath) { | |||
| if (this.inputPaths === undefined) { | |||
| var srcDir = this.inputPaths[0]; | ||
| var path = srcDir + '/' + relativePath; | ||
|
|
||
| return fs.lstatSync(path).isDirectory(); |
There was a problem hiding this comment.
rather then stating again, lets pass entry from https://github.com/stefanpenner/broccoli-persistent-filter/blob/master/index.js#L125 to getDestFilePath, that way we can simply ask it entry.isDirectory() etc.
| Filter.prototype.getDestFilePath = function(relativePath) { | ||
| // NOTE: relativePath may have been moved or unlinked | ||
| try { |
There was a problem hiding this comment.
with the above suggestion, i suspect this is no longer required.
|
@elucid sorry for letting this sit. Its hard to keep track of all this OSS stuff. I have left some comments, and will gladly help to make sure this lands promptly. |
|
addressed feedback and merged here: #109 |
getDestFilePathshould always returnnullfor directories.