lib: deprecate fd usage for fs.truncate and fs.truncateSync #15990

r1cebank · 2017-10-06T18:16:30Z

Address issue #15753

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

BridgeAR

We need a note in the deprecations.md about the new deprecation.

BridgeAR · 2017-10-06T23:43:33Z

lib/fs.js

@@ -783,6 +783,10 @@ fs.renameSync = function(oldPath, newPath) {

 fs.truncate = function(path, len, callback) {
  if (typeof path === 'number') {
+    process.emitWarning(
+      'Using fs.truncate with file descriptor deprecated. In the future, ' +
+      'use fs.ftruncate with file descriptor',


Nit - this should be indented with two more spaces while landing. The same applies to the other deprecation messages.

The message itself could also be improved by not saying In the future ... but just Please use ... instead.

I agree with switching to Please use .... Additionally, the occurrences of "with file descriptor" should likely be "with a file descriptor"

r1cebank · 2017-10-07T00:40:37Z

@cjihrig @BridgeAR I made some updates to the message and deprecations.md. Let me know if there is anything else I need to fix.

seishun · 2017-10-07T09:45:44Z

doc/api/deprecations.md

+`fs.truncate()` `fs.truncateSync()` usage with a file descriptor has been deprecated.
+
+*Note*: Please use `fs.ftruncate()` or `fs.ftruncateSync()` 
+if you need to work with file descriptors


"if you need" is redundant IMO. Also there should be a period at the end of the sentence.

cjihrig · 2017-10-07T19:40:58Z

doc/api/deprecations.md

@@ -719,6 +719,15 @@ The internal `path._makeLong()` was not intended for public use. However,
 userland modules have found it useful. The internal API has been deprecated
 and replaced with an identical, public `path.toNamespacedPath()` method.

+<a id="DEP0081"></a>


I think we're supposed to be using DEP00XX when authoring these deprecations. Then, when the PR is landed, the committer can replace it with a number.

seishun · 2017-10-07T19:51:04Z

lib/fs.js

@@ -783,6 +783,10 @@ fs.renameSync = function(oldPath, newPath) {

 fs.truncate = function(path, len, callback) {
  if (typeof path === 'number') {
+    process.emitWarning(


Please use util.deprecate() for runtime deprecations. See how it's done here.

I asked one of the mentor during the event, and looks like for this case since we are deprecating a particular usage, we cannot use util.deprecate().

Indeed. But we still need to make sure this warning is only emitted once and that it respects --no-deprecation. See how it's done with Buffer here (although that's a pending deprecation for now, so make sure to use process.noDeprecation instead of pendingDeprecation)

Oh I see what you mean, thank you for sharing the resources, I will make changes according to what you said.

jasnell · 2017-10-10T00:30:03Z

doc/api/fs.md

@@ -2268,6 +2268,8 @@ Asynchronous truncate(2). No arguments other than a possible exception are
 given to the completion callback. A file descriptor can also be passed as the
 first argument. In this case, `fs.ftruncate()` is called.

+*Note*: Passing a file descriptor is deprecated and may result in an error being thrown in the future.


Minor nit: this line is > 80 chars and will need to be wrapped before landing.

jasnell · 2017-10-10T00:31:37Z

lib/fs.js

@@ -63,6 +66,16 @@ const isWindows = process.platform === 'win32';
 const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
 const errnoException = util._errnoException;

+const doFlaggedDeprecation = noDeprecation ?
+  function() {} : showFlaggedDeprecation;


This part is unnecessary. The emitWarning function will handle the noDeprecation configuration property internally.

This was suggested by one of the reviewers, I am just here to learn how nodejs works internally. If you think emitWarning is sufficient, i can make the change.

jasnell · 2017-10-10T00:32:03Z

Ping @nodejs/tsc

r1cebank · 2017-10-10T00:37:40Z

Can someone give me a clear idea how this should be fixed? I feel all this back and forth commenting just made me more confused.

jasnell · 2017-10-10T00:54:23Z

@r1cebank ... first of all, thank you very much for the contribution, it really is quite appreciated. My apologies if the reviews were not clear. Essentially there are a couple of things here that should be reworked slightly.

Namely, I would change your code here:

const doFlaggedDeprecation = noDeprecation ?
   function() {} : showFlaggedDeprecation;
 
 function showFlaggedDeprecation() {
   process.emitWarning(
     'Using fs.truncate with a file descriptor is deprecated. Please use ' +
     'fs.ftruncate with a file descriptor instead.',
     'DeprecationWarning', 'DEP00XX');
 }

To the following:

let truncateWarn = true;
function doFlaggedDeprecation() {
  if (!truncateWarn) {
    process.emitWarning(
      'Using fs.truncate with a file descriptor is deprecated. Please use ' +
      'fs.ftruncate with a file descriptor instead.',
      'DeprecationWarning', 'DEP00XX');
    truncateWarn = false;
  }
}

There are two reasons:

Checking the noDeprecation flag in process.binding('config') is done automatically by emitWarning when DeprecationWarning is used. There is no need for you to do that here.
Adding the truncateWarn check ensures that the deprecation warning is only emitted once.

r1cebank · 2017-10-10T00:59:55Z

Thank you @jasnell for your wonderful explanation. I think I was little confused with the emit error only once part. Thanks for the code sample, this really helps with my understanding of Node.js. I will make these changes as soon as possible.

r1cebank · 2017-10-10T04:43:41Z

I made the changes @jasnell I hope this addressed all of your concerns.

addaleax · 2017-10-10T04:46:51Z

lib/fs.js

@@ -63,6 +63,18 @@ const isWindows = process.platform === 'win32';
 const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
 const errnoException = util._errnoException;

+let truncateWarn = true;
+
+function doFlaggedDeprecation() {


Just a suggestion, but I’d include truncate in the function name here because it’s specific to fs.truncate(Sync)

Very good suggestion, I will keep this in mind in my future works. Thanks

jasnell

Thank you!

trevnorris

missed a space in the doc change and the git message in the second commit is too long (max 50 chars). Thanks for the patch.

trevnorris · 2017-10-11T12:29:18Z

doc/api/deprecations.md

+Type: Runtime
+
+`fs.truncate()` `fs.truncateSync()` usage with
+a file descriptorhas been deprecated.


missed a space "descriptor has"

r1cebank · 2017-10-11T17:17:40Z

I have fixed both commit messages + missing space, thanks for the suggestion.

joyeecheung · 2017-10-12T03:56:07Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10634/

r1cebank · 2017-10-12T17:14:04Z

Anyone know if I can get read permission for jenkins? I cannot check the status of the build atm.

jasnell · 2017-10-13T00:48:17Z

Failure in CI is unrelated.

PR-URL: #15990 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2017-10-13T01:07:15Z

Landed in c885ea7

PR-URL: nodejs/node#15990 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 6, 2017

r1cebank changed the title ~~lib: deprecate fd usage for fs.truncate and fs.truncateSync #15753~~ lib: deprecate fd usage for fs.truncate and fs.truncateSync Oct 6, 2017

Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017

BridgeAR reviewed Oct 6, 2017

View reviewed changes

seishun reviewed Oct 7, 2017

View reviewed changes

cjihrig reviewed Oct 7, 2017

View reviewed changes

seishun added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 7, 2017

seishun reviewed Oct 7, 2017

View reviewed changes

jasnell reviewed Oct 10, 2017

View reviewed changes

jasnell requested a review from a team October 10, 2017 00:32

addaleax approved these changes Oct 10, 2017

View reviewed changes

jasnell approved these changes Oct 10, 2017

View reviewed changes

trevnorris approved these changes Oct 11, 2017

View reviewed changes

r1cebank added 2 commits October 11, 2017 10:13

lib: deprecate fd usage for fs.truncate(Sync)

927cf8d

doc: deprecation note for fs.truncate(Sync)

2ff66b9

cjihrig approved these changes Oct 13, 2017

View reviewed changes

jasnell closed this Oct 13, 2017

jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 13, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

benjamingr mentioned this pull request Nov 15, 2017

fs.truncate accepts an fd but it's poorly documented #15753

Closed

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: deprecate fd usage for fs.truncate and fs.truncateSync #15990

lib: deprecate fd usage for fs.truncate and fs.truncateSync #15990

r1cebank commented Oct 6, 2017 •

edited

Loading

BridgeAR left a comment

BridgeAR Oct 6, 2017

BridgeAR Oct 6, 2017

cjihrig Oct 6, 2017

r1cebank commented Oct 7, 2017 •

edited

Loading

seishun Oct 7, 2017

cjihrig Oct 7, 2017

seishun Oct 7, 2017

r1cebank Oct 7, 2017

seishun Oct 7, 2017

r1cebank Oct 7, 2017

jasnell Oct 10, 2017

jasnell Oct 10, 2017

r1cebank Oct 10, 2017

jasnell commented Oct 10, 2017

r1cebank commented Oct 10, 2017

jasnell commented Oct 10, 2017

r1cebank commented Oct 10, 2017

r1cebank commented Oct 10, 2017

addaleax Oct 10, 2017

r1cebank Oct 10, 2017

jasnell left a comment

trevnorris left a comment

trevnorris Oct 11, 2017

r1cebank commented Oct 11, 2017

joyeecheung commented Oct 12, 2017

r1cebank commented Oct 12, 2017

jasnell commented Oct 13, 2017

jasnell commented Oct 13, 2017

lib: deprecate fd usage for fs.truncate and fs.truncateSync #15990

lib: deprecate fd usage for fs.truncate and fs.truncateSync #15990

Conversation

r1cebank commented Oct 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

BridgeAR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r1cebank commented Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Oct 10, 2017

r1cebank commented Oct 10, 2017

jasnell commented Oct 10, 2017

r1cebank commented Oct 10, 2017

r1cebank commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

trevnorris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r1cebank commented Oct 11, 2017

joyeecheung commented Oct 12, 2017

r1cebank commented Oct 12, 2017

jasnell commented Oct 13, 2017

jasnell commented Oct 13, 2017

r1cebank commented Oct 6, 2017 •

edited

Loading

r1cebank commented Oct 7, 2017 •

edited

Loading