New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert "buffer: runtime deprecation of calling Buffer without new" #9529

Closed
wants to merge 1 commit into
base: master
from

Conversation

@addaleax
Member

addaleax commented Nov 9, 2016

  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer

Description of change

This reverts commit f2fe558 (#8169) as the original justification for the runtime-deprecation does not appear to justify the disruption to Node’s existing ecosystem. Futhermore, the possibility of deprecating the Buffer constructor entirely in v8.0 might lead to people having to change their code twice (For the record: While I opened this PR and that roadmap is one that some people have in mind, I personally don’t think the Buffer constructor should be deprecated entirely in v8.0).

/cc @nodejs/ctc

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Nov 9, 2016

Member

No hard feeling on this if we hard-deprecate it again soon together with new Buffer (where possible), but wouldn't it be too much distruption if we deprecate it in 7.0, undeprecate it in 7.x, then deprecate it again in 8.0? How are we going to message that? Upd: this is -0.

Member

ChALkeR commented Nov 9, 2016

No hard feeling on this if we hard-deprecate it again soon together with new Buffer (where possible), but wouldn't it be too much distruption if we deprecate it in 7.0, undeprecate it in 7.x, then deprecate it again in 8.0? How are we going to message that? Upd: this is -0.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

@ChALkeR I agree that that might be hard, but I think it is obvious that we need to come up with better messaging for this anyway. Whatever version we land a full runtime deprecation in (if we do that), we should make very clear both what we are doing and why we are doing it, in the changelog and over other channels.

@sindresorhus I’d be interested in hearing where your downvote is coming from. As you probably know, a lot of the push towards reverting the runtime deprecation is coming from people maintaining a noticeable share of the ecosystem, so I think it would really be interesting to hear whatever different perspective you may have?

Member

addaleax commented Nov 9, 2016

@ChALkeR I agree that that might be hard, but I think it is obvious that we need to come up with better messaging for this anyway. Whatever version we land a full runtime deprecation in (if we do that), we should make very clear both what we are doing and why we are doing it, in the changelog and over other channels.

@sindresorhus I’d be interested in hearing where your downvote is coming from. As you probably know, a lot of the push towards reverting the runtime deprecation is coming from people maintaining a noticeable share of the ecosystem, so I think it would really be interesting to hear whatever different perspective you may have?

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Nov 9, 2016

Contributor

I don't think we should do that. It's a sign of weakness to step back from the deprecation now, and v7 is a "unstable" release line for a reason. Better get the ecosystem in shape for v8, where it really matters.

Contributor

silverwind commented Nov 9, 2016

I don't think we should do that. It's a sign of weakness to step back from the deprecation now, and v7 is a "unstable" release line for a reason. Better get the ecosystem in shape for v8, where it really matters.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

It's a sign of weakness to step back from the deprecation now

I am not sure what you are getting at? You can see any actual listening to criticism as a sign of weakness if you want to, and I’m not sure why Node core should value appearing “strong” in the first place.

v7 is a "unstable" release line for a reason.

We’re not talking about experimental features here, and imho the main difference between odd and even release lines should be the different lifespan. The “Current” release line was previously called “Stable” for a reason, too, so I wouldn’t exactly jump to calling it “unstable” now. :)

Member

addaleax commented Nov 9, 2016

It's a sign of weakness to step back from the deprecation now

I am not sure what you are getting at? You can see any actual listening to criticism as a sign of weakness if you want to, and I’m not sure why Node core should value appearing “strong” in the first place.

v7 is a "unstable" release line for a reason.

We’re not talking about experimental features here, and imho the main difference between odd and even release lines should be the different lifespan. The “Current” release line was previously called “Stable” for a reason, too, so I wouldn’t exactly jump to calling it “unstable” now. :)

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Nov 9, 2016

Contributor

Okay, after reading up on #8169, I think I'll remain neutral here. I initialy thought this deprection was to push people towards the new APIs for security, but that issue clearly shows other intentions.

Contributor

silverwind commented Nov 9, 2016

Okay, after reading up on #8169, I think I'll remain neutral here. I initialy thought this deprection was to push people towards the new APIs for security, but that issue clearly shows other intentions.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

I initialy thought this deprection was to push people towards the new APIs for security, but that issue clearly shows other intentions.

Well, imho the impression you had is right – no deprecation should happen for any other reason than that one, and I don’t see any other arguments in favour of deprecating that are strong enough justify it (again, that’s my personal view).

Member

addaleax commented Nov 9, 2016

I initialy thought this deprection was to push people towards the new APIs for security, but that issue clearly shows other intentions.

Well, imho the impression you had is right – no deprecation should happen for any other reason than that one, and I don’t see any other arguments in favour of deprecating that are strong enough justify it (again, that’s my personal view).

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 9, 2016

Member

@addaleax I am in favour of deprecating APIs that suck, security or not, carefully, with appropriate timeline, I'm just not seeing how "not being an es6 class" really means "sucks", and I think having to type new everywhere makes the API more sucky... its no coincidence the node API (and most nice to use js APIs, AFAICT), try to make the new an implementation detail of a constructor.

The PRs mentioning es6 didn't work very hard to show what kinds of good things would be possible once Buffer is a "proper es6 class" that aren't possible now. The subtle interactions with some js array types that aren't used in the Node.js API anyhow (unlike Buffer) are also not clear. Probably this could be made compelling, but mostly ATM its just a bit mystifying.

Member

sam-github commented Nov 9, 2016

@addaleax I am in favour of deprecating APIs that suck, security or not, carefully, with appropriate timeline, I'm just not seeing how "not being an es6 class" really means "sucks", and I think having to type new everywhere makes the API more sucky... its no coincidence the node API (and most nice to use js APIs, AFAICT), try to make the new an implementation detail of a constructor.

The PRs mentioning es6 didn't work very hard to show what kinds of good things would be possible once Buffer is a "proper es6 class" that aren't possible now. The subtle interactions with some js array types that aren't used in the Node.js API anyhow (unlike Buffer) are also not clear. Probably this could be made compelling, but mostly ATM its just a bit mystifying.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

@sam-github Exactly, and I think our messaging turned out terrible here. As we discussed in today’s CTC meeting, we’re going to open a separate issue for more long-term focused planning, i.e. if, how, why and when to deprecate what exactly. I’ll do that, and try to provide everyone with information summing up what has happened so far, that just takes a bit of time.

Member

addaleax commented Nov 9, 2016

@sam-github Exactly, and I think our messaging turned out terrible here. As we discussed in today’s CTC meeting, we’re going to open a separate issue for more long-term focused planning, i.e. if, how, why and when to deprecate what exactly. I’ll do that, and try to provide everyone with information summing up what has happened so far, that just takes a bit of time.

@silverwind

This comment has been minimized.

Show comment
Hide comment
@silverwind

silverwind Nov 9, 2016

Contributor

its no coincidence the node API (and most nice to use js APIs, AFAICT), try to make the new an implementation detail of a constructor.

+1, I'm also annoyed when a dependency forces the new onto me. It can also lead to bugs when one does not know about the precedence of the new operator, e.g. new Thing().prop vs (new Thing()).prop, so if the new was the only reason for this change, I'd be in favor of reverting.

Contributor

silverwind commented Nov 9, 2016

its no coincidence the node API (and most nice to use js APIs, AFAICT), try to make the new an implementation detail of a constructor.

+1, I'm also annoyed when a dependency forces the new onto me. It can also lead to bugs when one does not know about the precedence of the new operator, e.g. new Thing().prop vs (new Thing()).prop, so if the new was the only reason for this change, I'd be in favor of reverting.

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 9, 2016

Member

Unless we are 100% that we want to actually do a runtime deprecation of Buffer without new in v8 I believe that this should be reverted. Even if we eventually decide to continue on the deprecation path, hopefully with the support of the community, we can add the warning back after giving individuals more time to get things in order. Alternatively if we find a way forward that does not involved deprecatio, then the warning is not going to be needed anyways.

LGTM

Member

MylesBorins commented Nov 9, 2016

Unless we are 100% that we want to actually do a runtime deprecation of Buffer without new in v8 I believe that this should be reverted. Even if we eventually decide to continue on the deprecation path, hopefully with the support of the community, we can add the warning back after giving individuals more time to get things in order. Alternatively if we find a way forward that does not involved deprecatio, then the warning is not going to be needed anyways.

LGTM

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Nov 9, 2016

Member

+0 on the revert itself, -1 on the messaging. We aren't doing this just because of the disruption, but because the justification for the hard-deprecation wasn't sufficient for it, and also because the plans to deprecate the Buffer constructor entirely in v8.0 might lead to people having to change their code twice.

Member

seishun commented Nov 9, 2016

+0 on the revert itself, -1 on the messaging. We aren't doing this just because of the disruption, but because the justification for the hard-deprecation wasn't sufficient for it, and also because the plans to deprecate the Buffer constructor entirely in v8.0 might lead to people having to change their code twice.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

@seishun I’ve update the commit message/PR content with bits of your comment, does that seem good to you?

Member

addaleax commented Nov 9, 2016

@seishun I’ve update the commit message/PR content with bits of your comment, does that seem good to you?

@seishun

This comment has been minimized.

Show comment
Hide comment
@seishun

seishun Nov 9, 2016

Member

@addaleax Still seems a bit off... Could it possibly be "too disruptive" while the justification is sufficient?

How about something like "as the original justification for the runtime-deprecation does not appear to justify the disruption to Node’s existing ecosystem"?

Member

seishun commented Nov 9, 2016

@addaleax Still seems a bit off... Could it possibly be "too disruptive" while the justification is sufficient?

How about something like "as the original justification for the runtime-deprecation does not appear to justify the disruption to Node’s existing ecosystem"?

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.
@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 9, 2016

Member

@seishun Yeah that sounds better to me, too.

Member

addaleax commented Nov 9, 2016

@seishun Yeah that sounds better to me, too.

@seishun

seishun approved these changes Nov 9, 2016

@targos

targos approved these changes Nov 9, 2016

@Trott

Trott approved these changes Nov 9, 2016

LGTM

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Nov 11, 2016

Member

Unless we are 100% that we want to actually do a runtime deprecation of Buffer without new in v8

I'm pretty sure that the right course of action for 8.0 would be to runtime-deprecate Buffer(arg), both with and without new, for security reasons.

I would ask to give this revert/discussion time at least before the end of the next CTC meeting before landing it.

Member

ChALkeR commented Nov 11, 2016

Unless we are 100% that we want to actually do a runtime deprecation of Buffer without new in v8

I'm pretty sure that the right course of action for 8.0 would be to runtime-deprecate Buffer(arg), both with and without new, for security reasons.

I would ask to give this revert/discussion time at least before the end of the next CTC meeting before landing it.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 16, 2016

Member

I would ask to give this revert/discussion time at least before the end of the next CTC meeting before landing it.

@ChALkeR If you’d like to talk about this in today’s/tomorrow’s meeting, I’d recommend announcing that in the meeting issue @ nodejs/CTC#33 (which I might very will have to miss).

Member

addaleax commented Nov 16, 2016

I would ask to give this revert/discussion time at least before the end of the next CTC meeting before landing it.

@ChALkeR If you’d like to talk about this in today’s/tomorrow’s meeting, I’d recommend announcing that in the meeting issue @ nodejs/CTC#33 (which I might very will have to miss).

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Nov 16, 2016

Member

My only question about this PR is this: if we (hopefully) decide to land full Buffer(arg) deprecation in 8.0 and do that, would we think that landing this PR was a good step, looking back from where we would be?

If yes — I have no objections. If no — we should have a resolution on the large issue (Buffer(arg)) deprecations first.

/cc @nodejs/ctc

Upd: in fact, tagging as ctc-agenda because of this.

Member

ChALkeR commented Nov 16, 2016

My only question about this PR is this: if we (hopefully) decide to land full Buffer(arg) deprecation in 8.0 and do that, would we think that landing this PR was a good step, looking back from where we would be?

If yes — I have no objections. If no — we should have a resolution on the large issue (Buffer(arg)) deprecations first.

/cc @nodejs/ctc

Upd: in fact, tagging as ctc-agenda because of this.

@ChALkeR ChALkeR added the ctc-agenda label Nov 16, 2016

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Nov 16, 2016

Member

In the last meeting we were converging on clean break > piecemeal deprecation, IIRC?

Member

bnoordhuis commented Nov 16, 2016

In the last meeting we were converging on clean break > piecemeal deprecation, IIRC?

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 21, 2016

Member

My only question about this PR is this: if we (hopefully) decide to land full Buffer(arg) deprecation in 8.0 and do that, would we think that landing this PR was a good step, looking back from where we would be?

I would say so. Judging from most of what I’ve been hearing, the deprecation warning as it is right now is not having the desired effect.

Member

addaleax commented Nov 21, 2016

My only question about this PR is this: if we (hopefully) decide to land full Buffer(arg) deprecation in 8.0 and do that, would we think that landing this PR was a good step, looking back from where we would be?

I would say so. Judging from most of what I’ve been hearing, the deprecation warning as it is right now is not having the desired effect.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Nov 21, 2016

Member

For absolute clarity: When @ChALkeR and @addaleax refer to "full Buffer(args) deprecation"/"full runtime deprecation", does that mean removal? Or does that mean returning to the runtime deprecation situation we have now? I've been assuming it meant removal. But I make a lot of bad assumptions...

Member

Trott commented Nov 21, 2016

For absolute clarity: When @ChALkeR and @addaleax refer to "full Buffer(args) deprecation"/"full runtime deprecation", does that mean removal? Or does that mean returning to the runtime deprecation situation we have now? I've been assuming it meant removal. But I make a lot of bad assumptions...

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Nov 21, 2016

Member

I interpreted “full Buffer(args) deprecation” as “print a deprecation warning for every call to Buffer(), with or without new and with any argument types”.

Member

addaleax commented Nov 21, 2016

I interpreted “full Buffer(args) deprecation” as “print a deprecation warning for every call to Buffer(), with or without new and with any argument types”.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Nov 21, 2016

Member

@Trott A single runtime deprecation when Buffer(arg… is used the first time, with or without new, if --no-deprecation flag was not set.

Member

ChALkeR commented Nov 21, 2016

@Trott A single runtime deprecation when Buffer(arg… is used the first time, with or without new, if --no-deprecation flag was not set.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Nov 21, 2016

Member

Ah! That answers a whole lot of questions I had. Thanks.

Member

Trott commented Nov 21, 2016

Ah! That answers a whole lot of questions I had. Thanks.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Nov 27, 2016

Member

Per nodejs/CTC#36, the CTC decision is to revert, so this should probably be landed before the next 7.x patch release.

Member

ChALkeR commented Nov 27, 2016

Per nodejs/CTC#36, the CTC decision is to revert, so this should probably be landed before the next 7.x patch release.

@Trott Trott removed the ctc-agenda label Nov 28, 2016

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Nov 28, 2016

Member

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.

Member

Trott commented 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.

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Nov 28, 2016

Member

Going to go ahead and land now. Thanks!

Member

evanlucas commented Nov 28, 2016

Going to go ahead and land now. Thanks!

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Nov 28, 2016

Member

Actually, running CI first to be safe: https://ci.nodejs.org/job/node-test-pull-request/5007/

Member

evanlucas commented Nov 28, 2016

Actually, running CI first to be safe: https://ci.nodejs.org/job/node-test-pull-request/5007/

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Nov 28, 2016

Member

Landed in 4fffe32. Thanks!

Member

evanlucas commented Nov 28, 2016

Landed in 4fffe32. Thanks!

@evanlucas evanlucas closed this Nov 28, 2016

evanlucas added a commit that referenced this pull request Nov 28, 2016

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.

PR-URL: #9529
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@addaleax addaleax deleted the addaleax:revert-8169 branch Nov 29, 2016

addaleax added a commit that referenced this pull request Dec 5, 2016

Revert "buffer: runtime deprecation of calling Buffer without new"
This reverts commit f2fe558
(#8169) as the original
justification for the runtime-deprecation does not appear
to justify the disruption to Node’s existing ecosystem.
Futhermore, the possibility of deprecating the Buffer constructor
entirely in v8.0 might lead to people having to change their code twice.

PR-URL: #9529
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

@Fishrock123 Fishrock123 referenced this pull request Dec 5, 2016

Merged

v7.2.1 proposal #10127

2 of 2 tasks complete

Fishrock123 added a commit that referenced this pull request Dec 6, 2016

2016-12-06, Version 7.2.1 (Current)
Notable changes:

* buffer:
  - Reverted the runtime deprecation of calling `Buffer()` without
`new`. (Anna Henningsen) #9529
  - Fixed `buffer.transcode()` for single-byte character
encodings to `UCS2`. (Anna Henningsen)
#9838
* promise: `--trace-warnings` now produces useful stacktraces for
Promise warnings. (Anna Henningsen)
#9525
* repl: Fixed a bug preventing correct parsing of generator functions.
(Teddy Katz) #9852
* V8: Fixed a significant `instanceof` performance regression.
(Franziska Hinkelmann) #9730

PR-URL: #10127

imyller added a commit to imyller/meta-nodejs that referenced this pull request Dec 7, 2016

2016-12-06, Version 7.2.1 (Current)
    Notable changes:

    * buffer:
      - Reverted the runtime deprecation of calling `Buffer()` without
    `new`. (Anna Henningsen) nodejs/node#9529
      - Fixed `buffer.transcode()` for single-byte character
    encodings to `UCS2`. (Anna Henningsen)
    nodejs/node#9838
    * promise: `--trace-warnings` now produces useful stacktraces for
    Promise warnings. (Anna Henningsen)
    nodejs/node#9525
    * repl: Fixed a bug preventing correct parsing of generator functions.
    (Teddy Katz) nodejs/node#9852
    * V8: Fixed a significant `instanceof` performance regression.
    (Franziska Hinkelmann) nodejs/node#9730

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

@joehand joehand referenced this pull request Dec 19, 2016

Closed

Deprecation warming #583

macedigital added a commit to macedigital/gulp-sri-hash that referenced this pull request Mar 13, 2017

test: prefer Buffer constructor over hacks to get back to 100% test c…
…overage (#2)

Revert version-check from previous commit 32604ba, and use Buffer constructor for converting HTML-formatted string. Looks like, nodejs isn't emitting a deprecation warning any more since v7.2.1 (nodejs/node#9529).

Mocking/Stubbing `process.version` is not only super ugly, but also super tricky. Until 8.x is released, no further changes should be necessary.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment