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.

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

Add the fs.mkdtemp() function. #5333

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
24 changes: 24 additions & 0 deletions doc/api/fs.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -522,6 +522,30 @@ to the completion callback. `mode` defaults to `0o777`.

Synchronous mkdir(2). Returns `undefined`.

## fs.mkdtemp(prefix, callback)

Creates a unique temporary directory.

Generates six random characters to be appended behind a required
`prefix` to create a unique temporary directory.

The created folder path is passed as a string to the callback's second
parameter.

Example:

```js
fs.mkdtemp('/tmp/foo-', (err, folder) => {
console.log(folder);
// Prints: /tmp/foo-itXde2
Copy link
Member

Choose a reason for hiding this comment

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

Indent error. You could put it on the same line as the console.log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See here as for why this indentation was chosen: #5333 (comment)

});
```

## fs.mkdtempSync(template)

The synchronous version of [`fs.mkdtemp()`][]. Returns the created
folder path.

Copy link
Member

Choose a reason for hiding this comment

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

Would prefer just duplicating the description of mkdtmp here rather than linking to it. Why make the user jump around?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because this is what every other sync function description does? I'm all for verboseness in documentation, but I'm even more in favor of consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I've been slowly working towards adding the verboseness. This is fine for now. We can expand it out later.

## fs.open(path, flags[, mode], callback)

Asynchronous file open. See open(2). `flags` can be:
Expand Down
21 changes: 21 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -2138,3 +2138,24 @@ 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');
}

if (!nullCheck(prefix, callback)) {
return;
}

var req = new FSReqWrap();
req.oncomplete = callback;

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

fs.mkdtempSync = function(prefix) {
nullCheck(prefix);

return binding.mkdtemp(prefix + 'XXXXXX');
};
26 changes: 26 additions & 0 deletions src/node_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,11 @@ static void After(uv_fs_t *req) {
static_cast<const uv_stat_t*>(req->ptr));
break;

case UV_FS_MKDTEMP:
argv[1] = String::NewFromUtf8(env->isolate(),
static_cast<const char*>(req->path));
break;

case UV_FS_READLINK:
argv[1] = String::NewFromUtf8(env->isolate(),
static_cast<const char*>(req->ptr));
Expand Down Expand Up @@ -1291,6 +1296,25 @@ static void FUTimes(const FunctionCallbackInfo<Value>& args) {
}
}

static void Mkdtemp(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);

if (args.Length() < 1)
return TYPE_ERROR("template is required");
if (!args[0]->IsString())
return TYPE_ERROR("template must be a string");

node::Utf8Value tmpl(env->isolate(), args[0]);

if (args[1]->IsObject()) {
ASYNC_CALL(mkdtemp, args[1], *tmpl);
} else {
SYNC_CALL(mkdtemp, *tmpl, *tmpl);
args.GetReturnValue().Set(String::NewFromUtf8(env->isolate(),
SYNC_REQ.path));
Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell Are we standardizing fs module to return utf8 encoded or one-byte encoded?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jasnell or would this run under your PR to allow an option to be passed to return a buffer instead of a string? guess either way it would return utf8 by default.

Copy link
Member

Choose a reason for hiding this comment

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

Yes and yes. If this lands first, I'll update #5616 to make this work that way. If #5616 lands first, this would need to be modified to work consistently.

}
}

void FSInitialize(const FunctionCallbackInfo<Value>& args) {
Local<Function> stats_constructor = args[0].As<Function>();
CHECK(stats_constructor->IsFunction());
Expand Down Expand Up @@ -1344,6 +1368,8 @@ void InitFs(Local<Object> target,
env->SetMethod(target, "utimes", UTimes);
env->SetMethod(target, "futimes", FUTimes);

env->SetMethod(target, "mkdtemp", Mkdtemp);

StatWatcher::Initialize(env, target);

// Create FunctionTemplate for FSReqWrap
Expand Down
27 changes: 27 additions & 0 deletions test/parallel/test-fs-mkdtemp.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
'use strict';

const common = require('../common');
const assert = require('assert');
const fs = require('fs');
const path = require('path');
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused import.

Copy link
Contributor

Choose a reason for hiding this comment

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

On line 10 and 16, path.join is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Back when he added the comment, it wasn't. Github just sucks at this kind of thing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Well, anyway, now we know this comment has been dealt with 👍

const Buffer = require('buffer').Buffer;

common.refreshTmpDir();

const tmpFolder = fs.mkdtempSync(path.join(common.tmpDir, 'foo.'));

assert(path.basename(tmpFolder).length === 'foo.XXXXXX'.length);
assert(common.fileExists(tmpFolder));
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a multi-byte character test. something like:

assert(Buffer.byteLength(path.basename(tmpFolder)) ===
  Buffer.byteLength('\0222abc.XXXXXX'));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean Buffer.byteLength('\u0222abc.XXXXXX')?

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes. missed the 'u'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. The test already runs fine on my computer, I'll try to find time to push this.


const utf8 = fs.mkdtempSync(path.join(common.tmpDir, '\u0222abc.'));
assert.equal(Buffer.byteLength(path.basename(utf8)),
Buffer.byteLength('\u0222abc.XXXXXX'));
assert(common.fileExists(utf8));

fs.mkdtemp(
path.join(common.tmpDir, 'bar.'),
common.mustCall(function(err, folder) {
assert.ifError(err);
assert(common.fileExists(folder));
})
);