This repository was archived by the owner on Nov 9, 2017. It is now read-only.
Repository navigation
This repository was archived by the owner on Nov 9, 2017. It is now read-only.
Call for vote on reverting runtime deprecation of using Buffer without new on v7 #36
Description
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;
Activity
addaleax commentedon Nov 23, 2016
+1
evanlucas commentedon Nov 23, 2016
+1
ChALkeR commentedon Nov 23, 2016
-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 commentedon Nov 23, 2016
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!)
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 commentedon Nov 23, 2016
Notes from today's CTC meeting on this are here and in #37.
ChALkeR commentedon Nov 23, 2016
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.
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 commentedon Nov 23, 2016
+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:
indutny commentedon Nov 23, 2016
👍
Fishrock123 commentedon Nov 23, 2016
Abstaining
ChALkeR commentedon Nov 23, 2016
@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 commentedon Nov 23, 2016
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 commentedon Nov 23, 2016
(I added a table indicating the status of the vote in the opening comment for this issue.)
cjihrig commentedon Nov 23, 2016
Abstain
mhdawson commentedon Nov 23, 2016
+1 to revert.
3 remaining items
bnoordhuis commentedon Nov 24, 2016
Abstaining. (I'll update the table.)
thefourtheye commentedon Nov 25, 2016
-1
ChALkeR commentedon Nov 25, 2016
Some stats on Buffer-without-new usage are here now!
Notes:
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
2016-10-22
2016-10-25
7.0.0 release
2016-11-24
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 commentedon Nov 26, 2016
@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 commentedon Nov 26, 2016
+1
rvagg commentedon Nov 27, 2016
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 commentedon Nov 27, 2016
@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 commentedon Nov 27, 2016
@rvagg I did not yet check the nested depencencies, but some points:
Buffer→new Bufferwithout caring about the usecase. We can file individual issues, though.Buffer-without-new usage — that reduced significantly.Buffer(argwith 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 commentedon Nov 28, 2016
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 commentedon Nov 28, 2016
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 commentedon Nov 28, 2016
@Fishrock123 nodejs/node#9529
evanlucas commentedon Nov 28, 2016
I'm going to go ahead and close as the vote passed and the revert has landed on master. Thanks!
trevnorris commentedon Nov 29, 2016
belated abstention. from the conversations I've had it seems this decision won't affect the long term API direction of Buffer.