fs: change default value of position in read and readSync

[RaisinTen]

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]

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]

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.

[RaisinTen]

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.

[aduh95]

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

[RaisinTen]

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

[jasnell]

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

[aduh95]

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

[RaisinTen]

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

[jasnell]

Likewise here, this check should be kept

[aduh95]

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]

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]

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]

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

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]

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]

need rebase to resolve git-conflicts

[RaisinTen]

@PoojaDurgad Thanks for mentioning, I rebased it.

[RaisinTen]

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]

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

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]

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

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]

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