Call for vote on reverting runtime deprecation of using Buffer without new on v7

[evanlucas]

Ref: nodejs/node#9529

/cc @nodejs/ctc

I would like for us to come to a resolution before next week if possible.

If you are in favor of reverting the change, please comment with +1
If you are in favor of leaving the change in, please comment with a -1
If you would like to abstain from the vote, please comment with abstain

---

VOTE STATUS TABLE

Vote	Count
In favor	7 (@addaleax @evanlucas @jasnell @indutny @mhdawson @thealphanerd @ofrobots)
Opposed	3 (@ChALkeR @mscdex @thefourtheye)
Abstain	6 (@Fishrock123 @Trott @cjihrig @shigeki @bnoordhuis @rvagg)
Total needed to pass/reject	7
Not yet voted	3 (@chrisdickinson @misterdjules @trevnorris)

Total needed to pass/reject is calculated with:

const ctcSize = 19;
return Math.floor((ctcSize - abstainVotes) / 2) + 1;

[addaleax]

+1

[evanlucas]

+1

[ChALkeR]

-1 for reverting now.

We don't have the current usage data (working on that), and I am uncertain how would this affect the future deprecation of Buffer(arg).

[Trott]

we don't have the current usage data

For the benefit of people who weren't at the meeting: This is data you are collecting and expect to have next week some time? (Correct me if I'm wrong about that, of course!)

I am uncertain how would this affect the future deprecation of Buffer(arg).

For clarity, is this a correct understanding?: You are opposed to removing this deprecation in version 7 if it means that we are not committed to a runtime deprecation in version 8. Removing in version 7 with a plan to put it back in version 8 might be OK. But removing and never putting it back is unacceptable.

[joshgav]

Notes from today's CTC meeting on this are here and in #37.

[ChALkeR]

This is data you are collecting and expect to have next week some time.

That is just the data on how the usage of Buffer(arg) without new has changed in the topmost packages — that would be available soon, hopefully in two days.

Taking dependencies into an account is harder, and that doesn't work yet in an automatic way, so it probably won't be ready next week.

You are opposed to removing this deprecation in version 7 if it means that we are not committed to a runtime deprecation in version 8. Removing in version 7 with a plan to put it back in version 8 might be OK. But removing and never putting it back is unacceptable.

Yes, that is correct. One notice, though — putting it back should have a «deprecated for security reasons» warning, a link to the explanation (on the reasons behind and how to fix things), and we probably should decide on the messaging of this before doing the revert in question here.

[jasnell]

+1 for reverting the current runtime deprecation. My preference would be to
replace with a new off-by-default deprecated-in-docs notice as suggested in
my recent pr.

On Wed, Nov 23, 2016 at 9:04 AM Evan Lucas notifications@github.com wrote:

Ref: nodejs/node#9529 nodejs/node#9529

/cc @nodejs/ctc https://github.com/orgs/nodejs/teams/ctc

I would like for us to come to a resolution before next week if possible.

If you are in favor of reverting the change, please comment with +1
If you are in favor of leaving the change in, please comment with a -1
If you would like to abstain from the vote, please comment with abstain

—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub
#36, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAa2eWBbp6_qlG8ZJDYPsMlEnM6pFktQks5rBHIzgaJpZM4K61SV
.

[indutny]

👍

[Fishrock123]

Abstaining

[ChALkeR]

@indutny, is that +1? (Just trying to be clear 😄).

Some old stats from a month ago: on 2016-10-22, there were approx. 1372 packages directly affected by this deprecation, 1127 were affected outside of the tests directories.

New stats (from today) will be available soon.

I can't tell the exact number of indirectly affected packages atm, though, but I'm going to do a rough estimation.

[Trott]

Abstaining, at least for now. I think this runtime deprecation should be rolled back for version 7, but I share @ChALkeR's concern that this might become a (bad) reason to never runtime deprecate the Buffer constructor ever. "We tried to deprecate it once before and it was horrible for everyone."

[Trott]

(I added a table indicating the status of the vote in the opening comment for this issue.)

[cjihrig]

Abstain

[mhdawson]

+1 to revert.

[MylesBorins]

+1 to revert

…

On Thu, Nov 24, 2016, 6:11 AM Michael Dawson ***@***.***> wrote: +1 to revert. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecVzUHV_oavGtBQX3rbZ-EBUYAyjlKks5rBLoBgaJpZM4K61SV> .

[mscdex]

-1

[shigeki]

abstain from this voting

[bnoordhuis]

Abstaining. (I'll update the table.)

[thefourtheye]

-1

[ChALkeR]

Some stats on Buffer-without-new usage are here now!

Notes:

• «total matches» and «total packages» do not affect end users, as those include matches in tests dirs, those affect only maintainers/authors of those packages.
• «integral downloads» doesn't take versions into an account (there are no per-version download stats from npm), so it counts only those packages which have their latest version affected.
• Doesn't include nested deps, only direct hits.
• «integral downloads» are normalized using the current downloads/month stats.
• There are false positives and false negatives, these stats are crude.
• The matches/packages counts were increasing or staying at the same level because of the new packages (npm grows quite fast), but old and popular packages migrate away from buffer-without-new, hence the integral downloads/month drops rapidly.

Taking that into an account, this still does provide some useful data. It could be seen that most part of the popular packages moved away from using buffer-without-new, but we have about 40% of the usage (counting by download stats) still there.

---

2016-08-04

• 4862 total matches
• 1020 total packages
• 2151 matches outside of test dirs
• 753 packages with matches outside of test dirs
• 29842894 integral downloads/month (outside of test dirs)

2016-10-22

• 5406 total matches
• 1110 total packages
• 2420 matches outside of test dirs
• 821 packages with matches outside of test dirs
• 19448511 integral downloads/month (outside of test dirs)

2016-10-25

7.0.0 release

2016-11-24

• 5399 total matches
• 1112 total packages
• 2328 matches outside of test dirs
• 816 packages with matches outside of test dirs
• 12385330 integral downloads/month (outside of test dirs)

---

Note: I updated the stats a bit, fixing some false positives and false negatives. I could be updating these in-place later without a further notice.

[addaleax]

@chrisdickinson @misterdjules @ofrobots @rvagg @trevnorris It would be great if you could indicate your preference or abstain from voting so that we have a resolution before the next release.

[ofrobots]

+1

[rvagg]

I have to abstain unfortunately, I have opinions but without having fully reviewed all of the (many) comments on this topic I don't feel it's responsible of me to weigh it either way!

[rvagg]

@ChALkeR does your vote get changed at all after looking at the stats? Does it sway you further in either way after you look at that?

[ChALkeR]

@rvagg I did not yet check the nested depencencies, but some points:

• This deprecation doesn't seem to be helping with the security issue, for the already mentioned reasons — it basically just forces a quick replacement Buffer → new Buffer without caring about the usecase. We can file individual issues, though.
• This deprecation does seem to be having a noticeable effect on Buffer-without-new usage — that reduced significantly.
• I am still unsure if reverting this deprecation will put an obstruction to runtime-deprecating Buffer(arg with or without new, also note the lack of understanding how this should be messaged — those are my main concerns here, and atm the sole reason for -1.

Nevertheless, the vote has already passed, 7 + 6/2 > 19/2, and the decision is to revert.

[Trott]

Appears to be resolution on the narrow issue here so I'm going to remove the ctc-agenda label. Feel free to re-add if this is something we need to talk about at the meeting this week.

[Fishrock123]

Is there a PR for this already? Someone will need to make it today to be able to release this week. Please do so ASAP.

[evanlucas]

@Fishrock123 nodejs/node#9529

[evanlucas]

I'm going to go ahead and close as the vote passed and the revert has landed on master. Thanks!

[trevnorris]

belated abstention. from the conversations I've had it seems this decision won't affect the long term API direction of Buffer.