buffer: use a default offset #19749

BridgeAR · 2018-04-02T12:34:21Z

If none is provided, use zero as a default offset for all read/write operations on the buffer.

This reverts some former changes to make sure the default offset for buffer read / write functions is set to zero. This was possible before due to the implicit type coercion.

I only added support for the functions that do not require a byteLength because writing e.g., buffer.readIntBe(undefined, 4) is very likely a bug in the users code and not expected to be zero.

~~At the same time I added a documentation-only deprecation for the default offset.~~ There were six modules with > 10dm that used this notation and three of those merged my fix already.

Refs: #18395

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

vsemozhetbyt · 2018-04-02T13:09:27Z

doc/api/deprecations.md

+
+Using a read or write functions without offset is very seldom useful besides by
+using it in the repl / for testing purposes. To make sure the intent is properly
+understood, please always add a offset instead of omitting it.


Nit: a offset -> an offset.

jasnell · 2018-04-02T13:53:29Z

semver-major by default unless TSC decides to go semver-minor with it.

addaleax · 2018-04-02T13:56:22Z

@jasnell Fyi, this is specifically fixing up an unintended breaking change from a previous semver-major commit. It brings behaviour back that exists in all release lines.

addaleax

lib/ and test/ LGTM, thanks!

addaleax · 2018-04-02T13:57:18Z

doc/api/deprecations.md

@@ -979,6 +979,15 @@ Type: Runtime
 This was an undocumented helper function not intended for use outside Node.js
 core and obsoleted by the removal of NPN (Next Protocol Negotiation) support.

+<a id="DEP00XX"></a>
+### DEP00XX: Buffer#(read|write) functions without offset


If you want to deprecate this, that should also be noted in the buffer.md docs.

However, I would suggest not deprecating this, or at least splitting that out into a different PR?

Splitting out to a separate PR would likely be best. It would, for instance, stop this from being a semver-major :-)

Yes, +1 to splitting the doc-deprecation to a separate PR so we could review the fix separately and without debating on the doc-deprecation.

I removed the deprecation as suggested.

BridgeAR · 2018-04-02T16:02:08Z

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

ChALkeR · 2018-04-04T09:53:59Z

@addaleax @jasnell Doesn't seem to be a semver-major anymore after the deprecation is removed from this PR. It still shouldn't be backported to 9.x and below though as it is a fix-up for an already landed semver-major patch.

ChALkeR · 2018-04-04T09:57:40Z

lib/internal/buffer.js

@@ -57,6 +57,8 @@ function boundsError(value, length, type) {

 // Read integers.
 function readUIntLE(offset, byteLength) {
+  if (offset === undefined)


buffer.readUIntLE() works on Node.js 9.x. Default offset is 0, default byteLength is 1.
Should this function be patched or is that set somewhere else?

Ah, good to know. I did not check that before. Even though this case is even a weirder use case... are we certain we want to support that? Right now there is not a single module that does this that has more than 10k downloads / month. That applies to all Buffer#(read|write)U?Int(L|B)E functions.

It’s an odd case, and it’s not ideal because it definitely doesn’t convey author intent very well, but if we do want to remove support for it, that should be a conscious decision (i.e. the default should be to keep it for now, imo)

Ok, so I'll add support for it first and then open a PR to suggest removing support for it? :D

@BridgeAR I think for behaviour that goes back this long (at least v0.12) and gave well-defined results, we should deprecate it if we do want to change runtime behaviour

But generally: Why? What’s the benefit of that? Yes, it’s good to convey author intent as well as possible when writing code, but taking care of that is just not our job… if we want people to have a canonical way to do something, that’s what documentation and docs-only deprecations are for.

@addaleax I just checked all npm js modules for this pattern and there is not a single hit for either no arguments or the first argument being undefined. It is very unlikely that anyone would want to call this function like this instead of calling Buffer#(read|write)U?Int8. I guess if this is ever called that way, it is actually a user bug and it is best to return an error instead. So I would like to keep it the way it is.

@BridgeAR Yeah, you’re right, this would most likely point to a user bug, and it seems fair to remove support as a semver-major change. (But stil, that should be a conscious decision that we’re making.)

ChALkeR · 2018-04-04T19:58:52Z

@BridgeAR Could you add a test to verify that buffer.readUIntLE() and buffer.readUIntLE(offset) are working as expected?

BridgeAR · 2018-04-04T20:12:51Z

@ChALkeR so if I understand you correct you do not agree with #19749 (comment)?

ChALkeR · 2018-04-04T20:21:17Z

@BridgeAR Removal or deprecation of that should be done in a separate PR with a separate discussion.
I would prefer this PR to just restore the offset behavior that we had in 9.x as deprecation/breakage of that behaviour was not properly discussed.

BridgeAR · 2018-04-05T16:00:59Z

@ChALkeR are you feeling strongly about first adding the support back in? In the end: it is nothing I am impacted and I do not want to invest to much energy into this but I believe we improve our user experience by keeping this PR as it is. I can only imagine this being a bug if a user will ever encounter this. When checking gzemnid I could also not find a single use case in any module (not only the top code but all modules).

BridgeAR · 2018-04-09T11:59:39Z

Ping @ChALkeR

BridgeAR · 2018-04-10T01:40:27Z

@ChALkeR I pushed another fixup to fully reinstate the default values as it was possible before (I still think any such use case is actually a bug). I hope the PR can now land out of your perspective.

BridgeAR · 2018-04-10T01:40:45Z

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

If none is provided, use zero as a default offset for all read/write operations on the buffer.

BridgeAR · 2018-04-10T02:05:49Z

Rebased due to conflicts about the former deprecations part.

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

BridgeAR · 2018-04-10T15:30:55Z

New CI (I had a regression): https://ci.nodejs.org/job/node-test-pull-request/14180/

BridgeAR · 2018-04-12T11:12:44Z

10.x is coming nearer each day, so I plan on landing this in ~30 hours if there is no objection until then.

jasnell · 2018-04-12T16:09:28Z

Even tho this is not semver-major, I'd feel better with a CITGM run on this.

BridgeAR · 2018-04-12T16:27:45Z

@jasnell this restores original behavior of 9.x, so there is no need to run the CITGM here.

jasnell · 2018-04-13T17:54:00Z

still LGTM.

If none is provided, use zero as a default offset for all read/write operations on the buffer. PR-URL: nodejs#19749 Refs: nodejs#18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR · 2018-04-13T17:58:51Z

Landed in d5495e8

If none is provided, use zero as a default offset for all read/write operations on the buffer. PR-URL: #19749 Refs: #18395 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

BridgeAR requested review from ChALkeR and addaleax April 2, 2018 12:34

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Apr 2, 2018

BridgeAR added dont-land-on-v6.x and removed buffer Issues and PRs related to the buffer subsystem. labels Apr 2, 2018

vsemozhetbyt reviewed Apr 2, 2018

View reviewed changes

jasnell approved these changes Apr 2, 2018

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 2, 2018

addaleax reviewed Apr 2, 2018

View reviewed changes

ChALkeR added this to the 10.0.0 milestone Apr 4, 2018

ChALkeR removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 4, 2018

ChALkeR reviewed Apr 4, 2018

View reviewed changes

ChALkeR added the dont-land-on-v4.x label Apr 4, 2018

addaleax approved these changes Apr 4, 2018

View reviewed changes

BridgeAR added 2 commits April 10, 2018 04:04

buffer: use a default offset

95449ef

If none is provided, use zero as a default offset for all read/write operations on the buffer.

fixup: accept weird cases as well

12e344a

BridgeAR force-pushed the accept-omitted-offset branch from 81b0d21 to 12e344a Compare April 10, 2018 02:04

ChALkeR self-requested a review April 10, 2018 02:38

fixup

b895a85

BridgeAR closed this Apr 13, 2018

ChALkeR removed their request for review April 13, 2018 18:12

BridgeAR deleted the accept-omitted-offset branch April 1, 2019 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: use a default offset #19749

buffer: use a default offset #19749

BridgeAR commented Apr 2, 2018 •

edited

Loading

vsemozhetbyt Apr 2, 2018

jasnell commented Apr 2, 2018

addaleax commented Apr 2, 2018

addaleax left a comment

addaleax Apr 2, 2018

jasnell Apr 2, 2018

ChALkeR Apr 2, 2018

BridgeAR Apr 2, 2018

BridgeAR commented Apr 2, 2018

ChALkeR commented Apr 4, 2018 •

edited

Loading

ChALkeR Apr 4, 2018 •

edited

Loading

BridgeAR Apr 4, 2018 •

edited

Loading

addaleax Apr 4, 2018

BridgeAR Apr 4, 2018

addaleax Apr 4, 2018

BridgeAR Apr 4, 2018

addaleax Apr 4, 2018

ChALkeR commented Apr 4, 2018

BridgeAR commented Apr 4, 2018 •

edited

Loading

ChALkeR commented Apr 4, 2018 •

edited

Loading

BridgeAR commented Apr 5, 2018

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 12, 2018

jasnell commented Apr 12, 2018

BridgeAR commented Apr 12, 2018

jasnell commented Apr 13, 2018

BridgeAR commented Apr 13, 2018

buffer: use a default offset #19749

buffer: use a default offset #19749

Conversation

BridgeAR commented Apr 2, 2018 • edited Loading

Checklist

Choose a reason for hiding this comment

jasnell commented Apr 2, 2018

addaleax commented Apr 2, 2018

addaleax 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

Choose a reason for hiding this comment

BridgeAR commented Apr 2, 2018

ChALkeR commented Apr 4, 2018 • edited Loading

ChALkeR Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

BridgeAR Apr 4, 2018 • 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

ChALkeR commented Apr 4, 2018

BridgeAR commented Apr 4, 2018 • edited Loading

ChALkeR commented Apr 4, 2018 • edited Loading

BridgeAR commented Apr 5, 2018

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 10, 2018

BridgeAR commented Apr 12, 2018

jasnell commented Apr 12, 2018

BridgeAR commented Apr 12, 2018

jasnell commented Apr 13, 2018

BridgeAR commented Apr 13, 2018

BridgeAR commented Apr 2, 2018 •

edited

Loading

ChALkeR commented Apr 4, 2018 •

edited

Loading

ChALkeR Apr 4, 2018 •

edited

Loading

BridgeAR Apr 4, 2018 •

edited

Loading

BridgeAR commented Apr 4, 2018 •

edited

Loading

ChALkeR commented Apr 4, 2018 •

edited

Loading