| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
If we ever decide to runtime-deprecate something, it should be the Buffer(string) overload, because that is the one that led to the issues with the Buffer constructor in the first place. |
Sorry, something went wrong.
|
@addaleax you mean to deprecate the usage of the buffer constructor with a single argument passed in? I also thought about that as only warning in case a number is passed is actually not what helps out anyone who regularly passes in a string and accepts user input for that. I am open for deprecating Buffer(string) in general, I am just not sure if that will find a majority of votes. |
Sorry, something went wrong.
|
@BridgeAR When I write Buffer(string) I mean the Buffer constructor with a single argument that has typeof arg === string :)
Both variants are common patterns, which is why I’m still -1 on runtime-deprecations in general.
I agree, it’s unlikely that this would help more than a handful people, if anyone. |
Sorry, something went wrong.
|
It won't persuade everyone, but I would be interested to know how much (if any) breakage a CITGM run with this causes compared to "full" deprecation. |
Sorry, something went wrong.
|
@Trott well, CITGM fails on master, so I doubt running it with this would be informative. |
Sorry, something went wrong.
|
This might be is almost certainly a terrible suggestion that will get support from neither side on this issue, but is there merit in considering "issue a deprecation warning when num is greater than <some large-ish value>"? |
Sorry, something went wrong.
|
@Trott ... that would not address the issues with the API. That is, the problem is not with Buffer(largeNumber) but Buffer(anyNumber) when it's not at all clear that anyNumber is actually a number or not ;-) |
Sorry, something went wrong.
@jasnell It addresses one of the issues with the API (the DoS issue).
Correct that it does not address the type-confusion issue. Looking at it through a the lens of people like @seishun who think a runtime deprecation is warranted, "It doesn't address everything" is not terribly persuasive if the only alternative is to address nothing at all. |
Sorry, something went wrong.
|
@jasnell Although thinking about it more...the DoS issue is only an issue with type confusion in the first place. And switching to Buffer.alloc() won't fix the DoS issue (although switching to Buffer.from() would). In any event, I do find it plausible that the cost of such a limited runtime deprecation may exceed its benefits. Hey, I did say it was probably a terrible idea. 🙃 |
Sorry, something went wrong.
|
@seishun Since 9.0.0 is out, should this be closed? Rebased and targeted for 10.0.0? Remain open pending something else ? |
Sorry, something went wrong.
|
If there is consensus that Buffer(num) won't be runtime-deprecated alone (aka either all forms of Buffer() will be deprecated together, or none) then I agree to close. |
Sorry, something went wrong.
|
Let us put it on the TSC agenda. There was not much discussion here, so I guess it will be best to force it a bit. That way we have more clarity. |
Sorry, something went wrong.
Ref: #15346 (comment) and subsequent conversation after that comment |
Sorry, something went wrong.
|
Since this is back on the agenda: /ping @nodejs/tsc @BridgeAR @seishun Is this the narrow question needing immediate attention?:
|
Sorry, something went wrong.
|
@Trott that looks good to me. |
Sorry, something went wrong.
My answer to that is no, because:
|
Sorry, something went wrong.
That's not entirely correct — on older Node.js versions, Buffer(num) allocates non-zero-filled arrays, so deprecating Buffer(num) would benefit the users of those old Node.js versions once they get new modules that migrate from Buffer(num) as a result of that deprecation. That said, I am not sure yet if it makes sense to get this in before deprecating Buffer(string). In fact, my current position is that both should be deprecated with 10.0 release (once 4.x goes unsupported). |
Sorry, something went wrong.
|
I would like to see this run against citgm |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This is a "light" version of #15346 that only prints the warning by default when [new ]Buffer(num) is used, as suggested by @BridgeAR in #15346 (comment).
Most reasons for runtime deprecation by default as listed in #15346 still apply here.
Checklist
Affected core subsystem(s)
buffer