fs: change default value of position in read and readSync by RaisinTen · Pull Request #37101 · nodejs/node

RaisinTen · 2021-01-27T13:40:14Z

Since the docs mention that position in read and readSync should
be an integer or a bigint, the functions should rather accept what
is actually fed into read, i.e., -1 instead of any nullish value.

RaisinTen · 2021-01-27T13:45:18Z

Not necessarily something to be done in this PR and I haven't looked super-closely but I wonder if this bit of code means that the docs shouldn't say the default is null but instead should say the default is -1 (and be sure to explain what -1 means and make sure that passing -1 explicitly works). Saying the thing has to be a an integer (or bigint, as added in this PR), but then saying the default is something that is not an integer or bigint (null), seems confusing and not quite right.

Originally posted by @Trott in #36190 (comment)

cc @Trott Since you mentioned this idea.

aduh95 · 2021-01-27T14:14:09Z

test/parallel/test-fs-readfile-fd.js


      // Read only five bytes, so that the position moves to five.
-      fs.read(fd, buf, 0, 5, null, common.mustSucceed((bytes) => {
+      fs.read(fd, buf, 0, 5, -1, common.mustSucceed((bytes) => {


Can you keep the null test please? We want null to still works – or mark the PR as semver-major.

EDIT: I've added the semver-major label, you can remove it if you change the code to rollback the behaviour for null.

I think this should be semver-major as it is supposed to reject a null value for position which was accepted previously. Thanks for adding the label.

On a second thought, we should probably deprecate it before removing it.

I added the deprecation warning, test and mentioned the PR link in the YAML. PTAL. 👀

jasnell · 2021-01-27T15:44:42Z

lib/fs.js


-  if (position == null)
-    position = -1;
-


We should actually keep this with if (position === null) given that the default assignment will not work when position is explicitly passed as null.

@jasnell Note this was intentional, see #37101 (comment) above.

@jasnell added the check back in along with a deprecation warning.

jasnell · 2021-01-27T15:45:04Z

lib/fs.js

  validateOffsetLengthRead(offset, length, buffer.byteLength);

-  if (position == null)
-    position = -1;


Likewise here, this check should be kept

test/parallel/test-fs-read-type.js

aduh95 · 2021-01-29T09:38:10Z

Usually we tend to avoid going Runtime deprecation without a doc-only deprecation first – unless there's a security risk we are trying to mitigate, but that's not the case here. You also need to add the deprecation description on deprecation.md.

Refs: https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#deprecations

RaisinTen · 2021-01-29T11:34:13Z

Okay. Should I open a separate PR to make it a doc-only deprecation first? After that lands, we can work on this PR to add the runtime warning. Finally, a PR can be raised to turn it into an EOL deprecation.

wa-Nadoo · 2021-01-29T16:45:13Z

The proposed default value for position seems confusing to me. Negative values feel like regular position values but in reverse order. Undefined/null seems more obvious.

RaisinTen · 2021-01-30T13:23:40Z

@wa-Nadoo -1 is actually the standard value for offset (or position from the side of Node.js) you pass to preadv2(https://man7.org/linux/man-pages/man2/preadv2.2.html) if you want the current file offset to be used and updated.

Trott · 2021-01-31T13:36:25Z

What's the reason for deprecating null? If it's an abstract sense of correctness, I would prefer to keep it around rather than break people's code for abstract ideals. On the other hand, if there is a true maintenance burden with keeping it, then sure, let's deprecate and remove.

RaisinTen · 2021-01-31T16:19:21Z

I intended to deprecate null because it previously allowed fs.read and fs.readSync to accept two different values for the same operation synthetically, by substituting null with -1 inside the functions since the underlying API uses -1. If the check is removed, the code will also become more efficient, which might be noticeable if there are lots of calls to these functions.

PoojaDurgad · 2021-03-29T08:14:42Z

need rebase to resolve git-conflicts

Since the docs mention that `position` in `read` and `readSync` should be an `integer` or a `bigint`, the functions should rather accept what is actually fed into `read`, i.e., `-1` instead of any nullish value.

RaisinTen · 2021-03-31T14:25:46Z

@PoojaDurgad Thanks for mentioning, I rebased it.

…ync" This reverts commit 44dfe5d.

Refs: nodejs#37101

RaisinTen · 2021-04-02T14:01:01Z

Usually we tend to avoid going Runtime deprecation without a doc-only deprecation first – unless there's a security risk we are trying to mitigate, but that's not the case here. You also need to add the deprecation description on deprecation.md.

Refs: https://github.com/nodejs/node/blob/master/doc/guides/collaborator-guide.md#deprecations

@aduh95 Finally opened the PR to doc-only deprecate this - #38043. :)

Note that I appreciate the time you spend working in this, but I think Rich has a point. Also if #37948 lands, I'd be -1 to land this.

RaisinTen · 2021-04-05T16:42:13Z

@aduh95 Thank you for the review.

Along with deprecating nullish values for position, I was also considering deprecating nullish values for offset. Though not documented, since offset is validated the same way as position, fs.read and fs.readSync actually allow the value null for offset too, which is not a valid integer.

Should documenting this be okay if #37948 lands?

Even if we document this, wouldn't it feel weird why the default value for offset is not null while that of position is? I'm assuming changing the default value is semver-major.

aduh95 · 2021-04-05T18:13:31Z

Changing a default is not necessarily semver-major, as long as it is not a breaking change.

Should documenting this be okay if #37948 lands?

IMHO changing the value in the docs makes sense, whether or not #37948 lands.

I don't have a strong opinion regarding deprecation status of nullish values, I tend to be on board for calling it legacy and keep it as is though.

RaisinTen · 2021-04-08T14:00:57Z

@aduh95 I noticed the validation logic is similar in fs Promises API. Would it make sense to make these validation changes to the FileHandle variations of these functions instead? The APIs will become a bit inconsistent with each other then.

aduh95 · 2021-04-08T21:00:36Z

I don't really have an opinion, I guess it would make sense to change all default values documented to -1 instead of null, but I don't feel strongly about it.

RaisinTen · 2022-03-18T06:32:00Z

I don't think this is worth the breakage anymore.

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jan 27, 2021

aduh95 reviewed Jan 27, 2021

View reviewed changes

aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 27, 2021

aduh95 previously approved these changes Jan 27, 2021

View reviewed changes

aduh95 mentioned this pull request Jan 27, 2021

fs: allow position parameter to be a BigInt in read and readSync #36190

Closed

4 tasks

jasnell reviewed Jan 27, 2021

View reviewed changes

RaisinTen force-pushed the fs/change-default-value-of-position-in-read branch from 70f5ff4 to 5145747 Compare January 28, 2021 15:41

aduh95 reviewed Jan 28, 2021

View reviewed changes

test/parallel/test-fs-read-type.js Outdated Show resolved Hide resolved

aduh95 added deprecations Issues and PRs related to deprecations. notable-change PRs with changes that should be highlighted in changelogs. labels Jan 29, 2021

RaisinTen mentioned this pull request Feb 2, 2021

doc: improve fsPromises docs #37186

Closed

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

fs: change default value of position in read and readSync

446bdb6

Since the docs mention that `position` in `read` and `readSync` should be an `integer` or a `bigint`, the functions should rather accept what is actually fed into `read`, i.e., `-1` instead of any nullish value.

RaisinTen force-pushed the fs/change-default-value-of-position-in-read branch from 6a815f7 to 446bdb6 Compare March 31, 2021 14:24

RaisinTen added 3 commits April 1, 2021 20:14

fixup! fs: change default value of position in read and readSync

44dfe5d

Revert "fixup! fs: change default value of position in read and readS…

42a31d1

…ync" This reverts commit 44dfe5d.

use default parameter

9342709

RaisinTen added a commit to RaisinTen/node that referenced this pull request Apr 2, 2021

fs,doc: doc-only deprecate nullish position in read and readSync

4ca8f33

Refs: nodejs#37101

RaisinTen mentioned this pull request Apr 2, 2021

fs,doc: doc-only deprecate nullish position in read and readSync #38043

Closed

RaisinTen closed this Mar 18, 2022

RaisinTen deleted the fs/change-default-value-of-position-in-read branch March 18, 2022 06:32

Uh oh!

Conversation

RaisinTen commented Jan 27, 2021

Uh oh!

RaisinTen commented Jan 27, 2021

Uh oh!

aduh95 Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 Jan 28, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 29, 2021

Choose a reason for hiding this comment

Uh oh!

jasnell Jan 27, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduh95 commented Jan 29, 2021

Uh oh!

RaisinTen commented Jan 29, 2021

Uh oh!

wa-Nadoo commented Jan 29, 2021

Uh oh!

RaisinTen commented Jan 30, 2021

Uh oh!

Trott commented Jan 31, 2021

Uh oh!

RaisinTen commented Jan 31, 2021

Uh oh!

PoojaDurgad commented Mar 29, 2021

Uh oh!

RaisinTen commented Mar 31, 2021

Uh oh!

RaisinTen commented Apr 2, 2021

Uh oh!

RaisinTen commented Apr 5, 2021

Uh oh!

aduh95 commented Apr 5, 2021

Uh oh!

RaisinTen commented Apr 8, 2021

Uh oh!

aduh95 commented Apr 8, 2021

Uh oh!

RaisinTen commented Mar 18, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduh95 Jan 27, 2021 •

edited

Loading