Add the fs.mkdtemp() function. #5333
Conversation
Trott
added
fs
|
Thanks for doing this! Before landing, this will need a test and a doc update. |
|
The commit log needs a description of what this adds and needs to be formatted according to the style guidelines... e.g. Question tho: aren't there userland modules already for this? |
|
There should be a |
|
Yes, the plan is to add the sync version, tests, and docs. Some of it is already on my HDD, but I wanted an early feedback on the idea itself, given how straightforward the code is. I won't be able to work on it this weekend though so that'll have to wait a bit :-) |
| @@ -1291,6 +1296,24 @@ static void FUTimes(const FunctionCallbackInfo<Value>& args) { | |||
| } | |||
| } | |||
|
|
|||
| static void Mkdtemp(const FunctionCallbackInfo<Value>& args) { | |||
| Environment* env = Environment::GetCurrent(args.GetIsolate()); | |||
There was a problem hiding this comment.
Environment::GetCurrent(args)
|
Needs documentation |
| @@ -2138,3 +2138,18 @@ SyncWriteStream.prototype.destroy = function() { | |||
| }; | |||
|
|
|||
| SyncWriteStream.prototype.destroySoon = SyncWriteStream.prototype.destroy; | |||
|
|
|||
| fs.mkdtemp = function(template, callback) { | |||
| if (typeof callback !== 'function') { | |||
There was a problem hiding this comment.
Any reason why this method raises a TypeError here rather than using maybeCallback?
There was a problem hiding this comment.
I reproduced the behavior of other fs methods.
|
There are now 3 commits in this PR:
|
| return folder.indexOf('foo.') === 0 && folder.length === 'foo.XXXXXX'.length; | ||
| })); | ||
|
|
||
| fs.mkdtemp(common.tmpDir + '/bar.XXXXXX', function(err, folder) { |
There was a problem hiding this comment.
You can use common.mustCall(function(..., so that it will fail the test if the callback is not invoked. Since we have the assert in the callback, it would be better to make sure that the callback got fired at least once.
There was a problem hiding this comment.
Also, don't ignore the error. You can do assert.ifError(err)
There was a problem hiding this comment.
Thanks for the feedback! I'll fix these.
ralt
force-pushed
the
mktemp
branch
2 times, most recently
from
3c7f26e
to
6ab77e5
Compare
|
I have fixed all the test/doc issues that were mentioned, thanks for the feedback! The only issue left I'm aware of is whether we want to force passing |
|
I'm not so keen on this, the argument is basically "other standard libraries have it therefore Node.js should" which we don't accept for so many other things. I understand the utility of such a function but I'm not buying the argument that core should subsume something because you don't like existing userland implementations -- go fix them if you don't like them. Also, note that Windows doesn't actually have this natively, it's not something universally understood to be an essential primitive, it could be built in userland using the primitives we already export from core. Also, the "it's in libuv therefore should be in Node.js" follows a similar logic and I don't subscribe to that either, just because you convince libuv to merge something doesn't mean we should automatically accept it here. |
Those are good points. The core has to provide just enough (and not more!) capabilities to userland implementations so that everything can be built on top of them in the JS side for the modules core supports (like fs, net and so on). The question we should be asking ourselves here at this point is: "Is this adding a new capability to Node or just something that users could built on top of existing stuff themselves?" - if the answer is "yes" then it should be added - if the answer is no this can be solved in userland. Here is the most popular userland solution https://www.npmjs.com/package/tmp (2m downloads). Pinging the author @raszi for information of any possible limitations the userland implementation has. |
|
I don't have a horse in this race, but I'll throw my 2 cents anyway: in this (and many other) case(s) the reason why libuv added a given feature was because Node wanted it, sort of. So there was a need for it, or at least some perceived it that way. To this day, the policy of what goes into core and what doesn't is not 100% clear to me. Adding this to userspace would require a binary addon, not sure if this should be considered or not. I also fail to understand why creating a directory is acceptable but creating a temporary directory is potentially not. The maintenance is virtually zero, since libuv does the actual work. As for the Windows part, it does have the ability to do so, but it doesn't provide the same guarantees as Unix, hence the hand rolled version. A quick look at node-temp shows that the responsibility for making multiple attempts in case of collision is pushed towards the user vs doing it automagically as libuv does. Another quick look at tmp shows that it generates a filename and checks if it exists with |
This uses libuv's mkdtemp function to provide a way to create a
temporary folder, using a prefix as the path. The prefix is appended
six random characters. The callback function will receive the name
of the folder that was created.
Usage example:
fs.mkdtemp('/tmp/foo-', function(err, folder) {
console.log(folder);
// Prints: /tmp/foo-Tedi42
});
The fs.mkdtempSync version is also provided. Usage example:
console.log(fs.mkdtemp('/tmp/foo-'));
// Prints: tmp/foo-Tedi42
This pull request also includes the relevant documentation changes
and tests.
PR-URL: #5333
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This uses libuv's mkdtemp function to provide a way to create a
temporary folder, using a prefix as the path. The prefix is appended
six random characters. The callback function will receive the name
of the folder that was created.
Usage example:
fs.mkdtemp('/tmp/foo-', function(err, folder) {
console.log(folder);
// Prints: /tmp/foo-Tedi42
});
The fs.mkdtempSync version is also provided. Usage example:
console.log(fs.mkdtemp('/tmp/foo-'));
// Prints: tmp/foo-Tedi42
This pull request also includes the relevant documentation changes
and tests.
PR-URL: #5333
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. (Forrest L Norvell) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)
Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)
Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344)
Notable changes: * buffer: * make byteLength work with ArrayBuffer & DataView (Jackson Tian) [#5255](#5255) * backport --zero-fill-buffers command line option (James M Snell) [#5744](#5744) * backport new buffer constructor APIs (James M Snell) [#5763](#5763) * add swap16() and swap32() methods (James M Snell) [#5724](#5724) * fs: add the fs.mkdtemp() function. (Florian MARGAINE) [#5333](#5333) * net: emit host in lookup event (HUANG Wei) [#5598](#5598) * node: --no-browser-globals configure flag (Fedor Indutny) [#5853](#5853) * npm: Upgrade to v3.8.3. Fixes a security flaw in the use of authentication tokens in HTTP requests that would allow an attacker to set up a server that could collect tokens from users of the command-line interface. Authentication tokens have previously been sent with every request made by the CLI for logged-in users, regardless of the destination of the request. This update fixes this by only including those tokens for requests made against the registry or registries used for the current install. (Forrest L Norvell) [npm#6](npm#6) * repl: support standalone blocks (Prince J Wesley) [#5581](#5581) * src: override v8 thread defaults using cli options (Tom Gallacher) [#4344](#4344) PR-URL: #5970
This uses libuv's mkdtemp function to provide a way to create a
temporary folder, using a prefix as the path. The prefix is appended
six random characters. The callback function will receive the name
of the folder that was created.
Usage example:
fs.mkdtemp('/tmp/foo-', function(err, folder) {
console.log(folder);
// Prints: /tmp/foo-Tedi42
});
The fs.mkdtempSync version is also provided. Usage example:
console.log(fs.mkdtemp('/tmp/foo-'));
// Prints: tmp/foo-Tedi42
This pull request also includes the relevant documentation changes
and tests.
PR-URL: #5333
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This uses libuv's mkdtemp function to provide a way to create a
temporary folder, using a prefix as the path. The prefix is appended
six random characters. The callback function will receive the name
of the folder that was created.
Usage example:
fs.mkdtemp('/tmp/foo-', function(err, folder) {
console.log(folder);
// Prints: /tmp/foo-Tedi42
});
The fs.mkdtempSync version is also provided. Usage example:
console.log(fs.mkdtemp('/tmp/foo-'));
// Prints: tmp/foo-Tedi42
This pull request also includes the relevant documentation changes
and tests.
PR-URL: #5333
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
This uses libuv's mkdtemp function to provide a way to create a
temporary folder, using a prefix as the path. The prefix is appended
six random characters. The callback function will receive the name
of the folder that was created.
Usage example:
fs.mkdtemp('/tmp/foo-', function(err, folder) {
console.log(folder);
// Prints: /tmp/foo-Tedi42
});
The fs.mkdtempSync version is also provided. Usage example:
console.log(fs.mkdtemp('/tmp/foo-'));
// Prints: tmp/foo-Tedi42
This pull request also includes the relevant documentation changes
and tests.
PR-URL: #5333
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796
Notable Changes: * child_process: add shell option to spawn() (cjihrig) #4598 * crypto: * add ALPN Support (Shigeki Ohtsu) #2564 * allow adding extra certs to well-known CAs (Sam Roberts) #9139 * deps: * v8: expose statistics about heap spaces (Ben Ripkens) #4463 * fs: add the fs.mkdtemp() function. (Florian MARGAINE) #5333 * process: * add `externalMemory` to `process` (Fedor Indutny) #9587 * add process.cpuUsage() (Patrick Mueller) #10796 PR-URL: #10973
Notable Changes:
* child_process: add shell option to spawn() (cjihrig)
nodejs/node#4598
* crypto:
* add ALPN Support (Shigeki Ohtsu)
nodejs/node#2564
* allow adding extra certs to well-known CAs (Sam Roberts)
nodejs/node#9139
* deps:
* v8: expose statistics about heap spaces (Ben Ripkens)
nodejs/node#4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
nodejs/node#5333
* process:
* add `externalMemory` to `process` (Fedor Indutny)
nodejs/node#9587
* add process.cpuUsage() (Patrick Mueller)
nodejs/node#10796
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Notable Changes:
* child_process: add shell option to spawn() (cjihrig)
nodejs/node#4598
* crypto:
* add ALPN Support (Shigeki Ohtsu)
nodejs/node#2564
* allow adding extra certs to well-known CAs (Sam Roberts)
nodejs/node#9139
* deps:
* v8: expose statistics about heap spaces (Ben Ripkens)
nodejs/node#4463
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
nodejs/node#5333
* process:
* add `externalMemory` to `process` (Fedor Indutny)
nodejs/node#9587
* add process.cpuUsage() (Patrick Mueller)
nodejs/node#10796
Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
Follow up on #5332
This PR only implements
mkdtempsince libuv only provides it. (cf. joyent/libuv#1368)Implementing
mkstempwould require having it in libuv first, so I guess this can wait. I'd argue that the most common use case is creating a temporary directory anyway.