Skip to content
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

Incorrect documentation or bug in Node #9165

Closed
mrchief opened this issue Oct 18, 2016 · 8 comments
Closed

Incorrect documentation or bug in Node #9165

mrchief opened this issue Oct 18, 2016 · 8 comments
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.

Comments

@mrchief
Copy link

mrchief commented Oct 18, 2016

Per this documentation: https://nodejs.org/dist/latest-v4.x/docs/api/buffer.html#buffer_class_method_buffer_from_array
image

Buffer.from(array) was added in v3.0. However, this example (taken from docs) fails in Node v4.2.3 and Node v4.4.4

const buf = Buffer.from([0x62,0x75,0x66,0x66,0x65,0x72]);
  // creates a new Buffer containing ASCII bytes
  // ['b','u','f','f','e','r']  <-- fails in Node < v4.5.0 


// error
TypeError: this is not a typed array.
   at Function.from (native)
   at repl:1:20
   at REPLServer.defaultEval (repl.js:262:27)
   at bound (domain.js:287:14)
   at REPLServer.runBound [as eval] (domain.js:300:12)
   at REPLServer.<anonymous> (repl.js:431:12)
   at emitOne (events.js:82:20)
   at REPLServer.emit (events.js:169:7)
   at REPLServer.Interface._onLine (readline.js:211:10)
   at REPLServer.Interface._line (readline.js:550:8)

Now as per latest docs: https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_array, our friend seems to have been "added in v5.10.0"

image

And rightfully so, fails in v5.0.0

λ rpm-extract → λ git master* → node -v
v5.0.0
λ rpm-extract → λ git master* → node
> console.log(Buffer.from([0x62,0x75,0x66,0x66,0x65,0x72]))
TypeError: this is not a typed array.
    at Function.from (native)
    at repl:1:20

So, my question is:

  • Documentation is inconsistent and misleading.
  • I want to support node v4 and v6. But this intermittent behavior will cause unwanted issues for consumers who are on > 4.0 and < 5.10.0 . What's the best way to handle this?

All the tests were done using nvm on a 64 bit OSX El Capitan.

@addaleax addaleax added doc Issues and PRs related to the documentations. v4.x labels Oct 18, 2016
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Oct 18, 2016
@addaleax
Copy link
Member

So, one thing: The docs for v4.x should say it was introduced in v4.5.0; before that, Buffer.from is just inherited from Uint8Array.from, which has only the from(array)

However, this example (taken from docs) fails in Node v4.2.3 and Node v4.4.4

That is because Uint8Array.from actually checks whether it was called as Uint8Array.from, not Buffer.from.

Documentation is inconsistent and misleading.

Yeah, that should be easy to fix.

What's the best way to handle this?

If you want/need to support versions below < 4.5 and < 5.10.0, you’ll likely have to stick with new Buffer() for now; otherwise, Buffer.from() should be okay.

@mrchief
Copy link
Author

mrchief commented Oct 18, 2016

Sounds good!

Out of curiosity, Buffer.from(Uint8Array.from([0x62,0x75,0x66,0x66,0x65,0x72])) should work across the board then? Will that be a performance sucker?

addaleax added a commit to addaleax/node that referenced this issue Oct 18, 2016
`Buffer.from` was present in `v3.0.0` but didn’t work for any
combination of arguments as it was inherited from `Uint8Array`.

This corrects the data to contain the version in which Node’s
own `Buffer.from` was added, namely `v4.5.0`.

Fixes: nodejs#9165
@addaleax
Copy link
Member

Out of curiosity, Buffer.from(Uint8Array.from([0x62,0x75,0x66,0x66,0x65,0x72])) should work across the board then?

Not for Node < 4.5.0, due to the way in which V8 implements Uint8Array.from. :/

@mrchief
Copy link
Author

mrchief commented Oct 18, 2016

Not for Node < 4.5.0, due to the way in which V8 implements Uint8Array.from. :/

Yeah. All of these fail (but different node versions complain differently)

Buffer.from(Uint8Array.from([0x62,0x75,0x66,0x66,0x65,0x72]))

Buffer.from(Uint8Array.from(new Set([0x62,0x75,0x66,0x66,0x65,0x72])))

So 2 things remain:

  • Why does Node v5 fail?
  • And new Buffer is deprecated in 6. I care for node v4 LTS and Node 6 only so thinking of this as a stop gap (until v4 support ends)
const buf = Buffer.from ? Buffer.from(arr) : new Buffer(arr)

@addaleax
Copy link
Member

Why does Node v5 fail?

Are you referring to the fact that v5.10.0 seems higher than v4.5.0, so it should incorporate features that are in all v4.x versions? Chronologically, v4.5.0 was released 5 months later than v5.10.0, and v4.x is still actively maintained. Occasionally, important new features like Buffer.from are backported there.

I care for node v4 LTS and Node 6 only so thinking of this as a stop gap (until v4 support ends)

Fwiw, there are a few polyfills available on npm. Also, you can just use new Buffer(arr) (but be sure to read the part of the Buffer docs that explain why it’s deprecated) – it doesn’t come with a runtime warning so far, and it’s quite possible that new Buffer() will never truly be broken.

@mrchief
Copy link
Author

mrchief commented Oct 18, 2016

Chronologically, v4.5.0 was released 5 months later than v5.10.0

I was referring to v5.0 (in my OP) but I guess this answers that as well.

Also, you can just use new Buffer(arr) (but be sure to read the part of the Buffer docs that explain why it’s deprecated)

For my current use case, I need new Buffer(arr). That seems pretty safe since I couldn't spot any warning related to that.

it’s quite possible that new Buffer() will never truly be broken
This settles it then! 😄

@addaleax
Copy link
Member

For my current use case, I need new Buffer(arr). That seems pretty safe since I couldn't spot any warning related to that.

No, as long as you know that the input type will always be the same, everything’s fine with using the old API – It’s when you accept input that could be controlled by somebody else (like from the browser) where things get potentially unsafe.

MylesBorins pushed a commit that referenced this issue Oct 24, 2016
`Buffer.from` was present in `v3.0.0` but didn’t work for any
combination of arguments as it was inherited from `Uint8Array`.

This corrects the data to contain the version in which Node’s
own `Buffer.from` was added, namely `v4.5.0`.

Fixes: #9165
PR-URL: #9167
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 26, 2016
`Buffer.from` was present in `v3.0.0` but didn’t work for any
combination of arguments as it was inherited from `Uint8Array`.

This corrects the data to contain the version in which Node’s
own `Buffer.from` was added, namely `v4.5.0`.

Fixes: #9165
PR-URL: #9167
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@targos
Copy link
Member

targos commented Jan 8, 2017

Fixed in #9167

@targos targos closed this as completed Jan 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

4 participants