fix(dest): don't modify original file object #20
Conversation
Instead of modifying the path of a file object when writing it to dest, construct a new File(file) from it so that the original object and upstream streams are not impacted. Closes #19
|
Should use file.clone not wrapping a new file. Can we consolidate all of this into one issue? It seems like this conversation is happening in 3 locations |
|
But we don't need to clone it...because gulp.dest isn't modifying the buffer. |
Copy own properties of original file so any custom properties that have been added by plugins are preserved.
|
I added a commit when I realized that there are plugins out there that assign custom properties to files. |
|
Seems like a flaw that would also make clone not copy custom properties |
|
True, but I still don't want to clone the file because I don't want to clone the buffer. |
|
@stu-salsbury Yup, just suggesting it as another potential issue. The code for copying the properties (and custom properties) should be in the same spot. Maybe we can make clone take an option to not copy the buffer/stream but just the rest of the properties. |
|
That sounds reasonable. If you give me the option name you want to use, I'm willing to take that on. However, the changes I made in vinyl-fs are still relevant, since it was actually testing whether the references were the same instead of the properties. If I implement the clone option I can commit again to this branch to fix the line in the dest function and keep the test changes. |
|
@stu-salsbury |
|
Ok, I will do this with support for custom properties, as well. I know you'll feel free to push back if you don't like it! :) |
| var oldFile = file; | ||
| file = new File(oldFile); | ||
| Object.keys(oldFile).forEach(function(key) { | ||
| if (Object.prototype.hasOwnProperty(key)) { |
There was a problem hiding this comment.
Shouldn't it be oldFile.hasOwnProperty(key) or the unnecessarily verbose equivalent Object.prototype.hasOwnProperty.call(oldFile, key)?
The way you're invoking the function directly from the prototype doesn't seem correct, as Object.prototype.hasOwnProperty(key) would check if key is an own property of Object.prototype.
Never mind this comment if that part will be replaced by file.clone()
Use vinyl clone with `contents: false` option to clone files before modifying their properties. Ensures upstream plugins do not see changes to properties that result from modifying their destination. Closes #19
|
new commit -- will only work properly when corresponding PR is merged in vinyl: gulpjs/vinyl#15 |
|
So I'm torn on this one - in the guidelines we do specify that plugins need to make sure they don't emit the file before they are finished with it. Do you know which plugin is responsible or is it a lazypipe issue? |
|
ping ^ |
|
Closing due to no response |
Instead of modifying the path of a file object when writing it to dest, construct a new File(file) from it so that the original object and upstream streams are not impacted.
Closes #19
The major issue with this change was that some of the unit tests were testing whether the file that came out was the same object as the one that came in, instead of actually verifying that the properties on the outgoing file were correct. So I think I've improved the tests along the way.