New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix empty second request due BufferStream chunks reference colateral #161
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flushing the buffer was the intended API, and might be depended upon. It would be unwise to change the behavior without a major version bump, but it is your call.
…avoid side effect on construcor
@kriskowal How about 469ff2c it should not change forEach behavior. |
Nah breaking test, will look why previous change did not. |
Here we go, restored BufferStream and only fixed fsMock, tests are passing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works and nits.
fs-mock.js
Outdated
@@ -126,7 +126,9 @@ MockFs.prototype.open = function (path, flags, charset, options) { | |||
charset | |||
); | |||
} else { | |||
return new BufferStream(fileNode._chunks, charset); | |||
// Clone chunks to avoid side effect | |||
var bufferChunks = fileNode._chunks.slice(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idiom is slice() without the implicit 0 arg.
Could also add a BufferStream.prototype.clone() to do less reaching through the encapsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fixed slice()
I prefer doing minor change, I can do a separate push for BufferStream.prototype.clone.
Just to validate that what you meant:
diff --git i/buffer-stream.js w/buffer-stream.js
index 19184cc..24c356b 100644
--- i/buffer-stream.js
+++ w/buffer-stream.js
@@ -3,7 +3,7 @@ var Q = require("q");
var Reader = require("./reader");
module.exports = BufferStream;
-function BufferStream(chunks, charset) {
+function BufferStream(chunks, charset, close) {
if (!(this instanceof BufferStream)) {
return new BufferStream(chunks, charset);
}
@@ -14,7 +14,7 @@ function BufferStream(chunks, charset) {
}
this._charset = charset;
this._chunks = chunks;
- this._close = Q.defer();
+ this._close = close || Q.defer();
this.closed = this._close.promise;
}
@@ -35,6 +35,11 @@ BufferStream.prototype.read = function () {
return Q.resolve(result);
};
+BufferStream.prototype.clone = function () {
+ var clone = BufferStream(this._chunks.slice(), this._charset, this._close);
+ return Q.resolve(clone);
+};
+
BufferStream.prototype.write = function (chunk) {
if (this._charset) {
chunk = new Buffer(String(chunk), this._charset);
diff --git i/fs-mock.js w/fs-mock.js
index 84116c6..316fc5c 100644
--- i/fs-mock.js
+++ w/fs-mock.js
@@ -126,9 +126,7 @@ MockFs.prototype.open = function (path, flags, charset, options) {
charset
);
} else {
- // Clone chunks to avoid side effect
- var bufferChunks = fileNode._chunks.slice();
- return new BufferStream(bufferChunks, charset);
+ return new BufferStream(bufferChunks, charset).clone();
}
}
});
@kriskowal if you agreed, merge. |
I’ve granted you collaborator permissions and approved the request. You are free to merge, cut a patch, and release. (I don’t want to block you) I will add you as an owner in npm as well. Is this you? https://www.npmjs.com/~hthetiot |
@kriskowal Yes I'm ~hthetiot on npm. |
|
fix empty File.chunks on MockFs second request due BufferStream splice on reference.
Using following example with Joey, "a/b/d.txt" get only served once:
See example montagejs/joey#16