Skip to content
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.

Sign up for GitHub

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

fs: add Buffer support in fs methods #5616

Closed
wants to merge 3 commits into from
Closed

fs: add Buffer support in fs methods #5616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc12d84
doc: general improvements to fs.markdown
jasnell Mar 18, 2016
f4cfa60
fs: Buffer and encoding enhancements to fs API
jasnell Mar 9, 2016
9272f05
fs: make fs.watch error message more useful
jasnell Mar 18, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
385 changes: 360 additions & 25 deletions doc/api/fs.markdown
View file
Open in desktop

Large diffs are not rendered by default.

111 changes: 83 additions & 28 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,17 +903,32 @@ fs.mkdirSync = function(path, mode) {
modeNum(mode, 0o777));
};

fs.readdir = function(path, callback) {
fs.readdir = function(path, options, callback) {
options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
}
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');

callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
binding.readdir(pathModule._makeLong(path), req);
binding.readdir(pathModule._makeLong(path), options.encoding, req);
};

fs.readdirSync = function(path) {
fs.readdirSync = function(path, options) {
options = options || {};
if (typeof options === 'string')
options = {encoding: options};
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
nullCheck(path);
return binding.readdir(pathModule._makeLong(path));
return binding.readdir(pathModule._makeLong(path), options.encoding);
};

fs.fstat = function(fd, callback) {
Expand Down Expand Up @@ -952,17 +967,31 @@ fs.statSync = function(path) {
return binding.stat(pathModule._makeLong(path));
};

fs.readlink = function(path, callback) {
fs.readlink = function(path, options, callback) {
options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
}
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
callback = makeCallback(callback);
if (!nullCheck(path, callback)) return;
var req = new FSReqWrap();
req.oncomplete = callback;
binding.readlink(pathModule._makeLong(path), req);
binding.readlink(pathModule._makeLong(path), options.encoding, req);
};

fs.readlinkSync = function(path) {
fs.readlinkSync = function(path, options) {
options = options || {};
if (typeof options === 'string')
options = {encoding: options};
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
nullCheck(path);
return binding.readlink(pathModule._makeLong(path));
return binding.readlink(pathModule._makeLong(path), options.encoding);
};

function preprocessSymlinkDestination(path, type, linkPath) {
Expand Down Expand Up @@ -1353,7 +1382,10 @@ function FSWatcher() {
this._handle.onchange = function(status, event, filename) {
if (status < 0) {
self._handle.close();
const error = errnoException(status, `watch ${filename}`);
const error = !filename ?
errnoException(status, 'Error watching file for changes:') :
errnoException(status,
`Error watching file ${filename} for changes:`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's unrelated to the Buffer / encoding change but given that the current error message is fairly useless when filename is not returned, it certainly seems worthwhile since I've got my hands in this code anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just break it out into its own commit? May be small, but in case one needs to be rolled back for some unknown reason it won't affect the other.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this has been broken out into a separate commit along with many of the other non-directly related cleanups to fs.markdown that are included.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my understanding, this will render as Error watching <Buffer ..> for changes: when encoding == 'buffer'?

error.filename = filename;
self.emit('error', error);
} else {
Expand All @@ -1363,11 +1395,15 @@ function FSWatcher() {
}
util.inherits(FSWatcher, EventEmitter);

FSWatcher.prototype.start = function(filename, persistent, recursive) {
FSWatcher.prototype.start = function(filename,
persistent,
recursive,
encoding) {
nullCheck(filename);
var err = this._handle.start(pathModule._makeLong(filename),
persistent,
recursive);
recursive,
encoding);
if (err) {
this._handle.close();
const error = errnoException(err, `watch ${filename}`);
Expand All @@ -1380,25 +1416,27 @@ FSWatcher.prototype.close = function() {
this._handle.close();
};

fs.watch = function(filename) {
fs.watch = function(filename, options, listener) {
nullCheck(filename);
var watcher;
var options;
var listener;

if (arguments[1] !== null && typeof arguments[1] === 'object') {
options = arguments[1];
listener = arguments[2];
} else {
options = options || {};
if (typeof options === 'function') {
listener = options;
options = {};
listener = arguments[1];
} else if (typeof options === 'string') {
options = {encoding: options};
}
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');

if (options.persistent === undefined) options.persistent = true;
if (options.recursive === undefined) options.recursive = false;

watcher = new FSWatcher();
watcher.start(filename, options.persistent, options.recursive);
const watcher = new FSWatcher();
watcher.start(filename,
options.persistent,
options.recursive,
options.encoding);

if (listener) {
watcher.addListener('change', listener);
Expand Down Expand Up @@ -2139,10 +2177,19 @@ SyncWriteStream.prototype.destroy = function() {

SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy;

fs.mkdtemp = function(prefix, callback) {
if (typeof callback !== 'function') {
throw new TypeError('"callback" argument must be a function');
fs.mkdtemp = function(prefix, options, callback) {
if (!prefix || typeof prefix !== 'string')
throw new TypeError('filename prefix is required');

options = options || {};
if (typeof options === 'function') {
callback = options;
options = {};
} else if (typeof options === 'string') {
options = {encoding: options};
}
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');

if (!nullCheck(prefix, callback)) {
return;
Expand All @@ -2151,11 +2198,19 @@ fs.mkdtemp = function(prefix, callback) {
var req = new FSReqWrap();
req.oncomplete = callback;

binding.mkdtemp(prefix + 'XXXXXX', req);
binding.mkdtemp(prefix + 'XXXXXX', options.encoding, req);
};

fs.mkdtempSync = function(prefix) {
fs.mkdtempSync = function(prefix, options) {
if (!prefix || typeof prefix !== 'string')
throw new TypeError('filename prefix is required');

options = options || {};
if (typeof options === 'string')
options = {encoding: options};
if (typeof options !== 'object')
throw new TypeError('"options" must be a string or an object');
nullCheck(prefix);

return binding.mkdtemp(prefix + 'XXXXXX');
return binding.mkdtemp(prefix + 'XXXXXX', options.encoding);
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noting that the type checks for options will let through null, which will make options.encoding fail with a TypeError. GIGO, IMO - I'll leave it up to you if you want to handle that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options = options || {} a few lines above should catch that.

27 changes: 22 additions & 5 deletions src/fs_event_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "util-inl.h"
#include "node.h"
#include "handle_wrap.h"
#include "string_bytes.h"

#include <stdlib.h>

Expand Down Expand Up @@ -41,6 +42,7 @@ class FSEventWrap: public HandleWrap {

uv_fs_event_t handle_;
bool initialized_;
enum encoding encoding_;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: if encoding_ never changes, make it const.

EDIT: Never mind, I see that it gets assigned after construction.

};


Expand Down Expand Up @@ -86,16 +88,20 @@ void FSEventWrap::Start(const FunctionCallbackInfo<Value>& args) {

FSEventWrap* wrap = Unwrap<FSEventWrap>(args.Holder());

if (args.Length() < 1 || !args[0]->IsString()) {
return env->ThrowTypeError("filename must be a valid string");
}
static const char kErrMsg[] = "filename must be a string or Buffer";
if (args.Length() < 1)
return env->ThrowTypeError(kErrMsg);

node::Utf8Value path(env->isolate(), args[0]);
BufferValue path(env->isolate(), args[0]);
if (*path == nullptr)
return env->ThrowTypeError(kErrMsg);

unsigned int flags = 0;
if (args[2]->IsTrue())
flags |= UV_FS_EVENT_RECURSIVE;

wrap->encoding_ = ParseEncoding(env->isolate(), args[3], UTF8);

int err = uv_fs_event_init(wrap->env()->event_loop(), &wrap->handle_);
if (err == 0) {
wrap->initialized_ = true;
Expand Down Expand Up @@ -156,7 +162,18 @@ void FSEventWrap::OnEvent(uv_fs_event_t* handle, const char* filename,
};

if (filename != nullptr) {
argv[2] = OneByteString(env->isolate(), filename);
Local<Value> fn = StringBytes::Encode(env->isolate(),
filename,
wrap->encoding_);
if (fn.IsEmpty()) {
argv[0] = Integer::New(env->isolate(), UV_EINVAL);
argv[2] = StringBytes::Encode(env->isolate(),
filename,
strlen(filename),
BUFFER);
} else {
argv[2] = fn;
}
}

wrap->MakeCallback(env->onchange_string(), ARRAY_SIZE(argv), argv);
Expand Down
Loading