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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

r1cebank
Copy link

@r1cebank r1cebank commented Oct 6, 2017

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)

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 6, 2017
@r1cebank 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 Trott added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Oct 6, 2017
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

lib/fs.js Outdated
@@ -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',
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@r1cebank
Copy link
Author

r1cebank commented Oct 7, 2017

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

`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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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 seishun added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 7, 2017
lib/fs.js Outdated
@@ -783,6 +783,10 @@ fs.renameSync = function(oldPath, newPath) {

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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().

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Author

Choose a reason for hiding this comment

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

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

doc/api/fs.md Outdated
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

lib/fs.js Outdated
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Member

jasnell commented Oct 10, 2017

Ping @nodejs/tsc

@jasnell jasnell requested a review from a team October 10, 2017 00:32
@r1cebank
Copy link
Author

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
Copy link
Member

jasnell commented Oct 10, 2017

@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:

  1. 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.
  2. Adding the truncateWarn check ensures that the deprecation warning is only emitted once.

@r1cebank
Copy link
Author

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
Copy link
Author

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

lib/fs.js Outdated
@@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

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.

Type: Runtime

`fs.truncate()` `fs.truncateSync()` usage with
a file descriptorhas been deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

missed a space "descriptor has"

@r1cebank
Copy link
Author

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

@joyeecheung
Copy link
Member

@r1cebank
Copy link
Author

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

@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Failure in CI is unrelated.

jasnell pushed a commit that referenced this pull request Oct 13, 2017
PR-URL: #15990
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@jasnell
Copy link
Member

jasnell commented Oct 13, 2017

Landed in c885ea7

@jasnell jasnell closed this Oct 13, 2017
@jasnell jasnell added the notable-change PRs with changes that should be highlighted in changelogs. label Oct 13, 2017
addaleax pushed a commit to ayojs/ayo that referenced this pull request Oct 15, 2017
PR-URL: nodejs/node#15990
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@tniessen tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. deprecations Issues and PRs related to deprecations. fs Issues and PRs related to the fs subsystem / file system. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.