buffer: support boxed strings and toPrimitive #13725

jasnell · 2017-06-16T19:38:03Z

Add support for Buffer.from(new String('...')) and Buffer.from({[Symbol.toPrimitive]() { return '...'; }})

const buf1 = Buffer.from(new String('this is a test'));

class Foo {
  [Symbol.toPrimitive]() {
    return 'this is a test';
  }
}
const buf2 = Buffer.from(new Foo());

/cc @bnoordhuis @addaleax

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

buffer

mscdex · 2017-06-16T19:51:09Z

lib/buffer.js

@@ -170,18 +170,31 @@ Object.defineProperty(Buffer, Symbol.species, {
 * Buffer.from(arrayBuffer[, byteOffset[, length]])
 **/
 Buffer.from = function(value, encodingOrOffset, length) {
+  if (value === undefined || value === null)


== null ?

mscdex · 2017-06-16T19:53:07Z

lib/buffer.js

+
+  if (typeof value === 'number')
+    throw new TypeError('"value" argument must not be a number');
+
  if (typeof value === 'string')


IMHO we should be promoting this and the other more common cases to the top of this function. This is why typeof value === 'number' was at the very end for example.

yeah, good point, I updated and shifted the number check back down a bit. It cannot be at the bottom like before because doing so causes the valueOf option to go into a recursion loop.

BridgeAR · 2017-06-16T20:45:25Z

lib/buffer.js

+    throw new TypeError(kFromErrorMsg);
+
+  if (typeof value === 'number')
+    throw new TypeError('"value" argument must not be a number');


I guess it would be good to use internal/errors here.

I'm going to be updating the buffer errors all at once very soon.

mscdex · 2017-06-16T21:05:03Z

lib/buffer.js

+  if (typeof value === 'number')
+    throw new TypeError('"value" argument must not be a number');
+
+  const valueOf = value.valueOf();


I wonder if we should be more cautious here or not, in case someone passes say Object.create(null). Perhaps value.valueOf && value.valueOf() at the very least ?

addaleax · 2017-06-16T20:55:23Z

lib/buffer.js

-    throw new TypeError('"value" argument must not be a number');
+  if (typeof value === 'object' &&
+      typeof value[Symbol.toPrimitive] === 'function') {
+    return Buffer.from(value[Symbol.toPrimitive](), encodingOrOffset, length);


We should call value[Symbol.toPrimitive] with a hint argument of 'string', I would say

mscdex · 2017-06-16T21:16:22Z

lib/buffer.js

+  const valueOf = value.valueOf();
+  if (valueOf !== value)
+    return Buffer.from(valueOf, encodingOrOffset, length);
+
  var b = fromObject(value);


Now that we're supposedly no longer bound by Crankshaft's inlining limitations, perhaps we might also move the isUint8Array() case from fromObject() into this function, right below the isAnyArrayBuffer() case?

I'm updating to handle the other suggestions, I'll look at this one separately.

Fishrock123 · 2017-06-16T22:05:58Z

Is there any real use to supporting new String('...')? I think we should try to dissuade users from using Primitive-Objects when possible.

refack · 2017-06-16T23:02:20Z

doc/api/buffer.md

+
+```js
+const buf = Buffer.from(new String('this is a test'));
+```


Maybe it's just me, but I'm missing the outcome. Like:

buf.toString() === 'this is a test'

Or even:

console.log(buf)

outputs:

<Buffer 74 68 69 73 20 69 73 20 61 20 74 65 73 74>

Not sure I understand your point here.

You show the correct usage, but not the expected outcome. It feels incomplete, but I don't have a strong opinion.

Except something @addaleax wrote somewhere, maybe we'll be able to run the snippets in the docs as a sort of acceptance test. For that we need some assertion on outcomes.

refack · 2017-06-16T23:13:56Z

lib/buffer.js

  var b = fromObject(value);
  if (b)
    return b;

-  if (typeof value === 'number')
-    throw new TypeError('"value" argument must not be a number');
+  if (typeof value === 'object' &&


tiny nit: you can try to access properties on any !(value === undefined || value === null) value so maybe just (typeof value[Symbol.toPrimitive] === 'function')?

jasnell · 2017-06-17T00:22:34Z

@Fishrock123 ... it's more about principle of least-surprise. The lack of support for new String() surprised at least one user recently #13652. It's the kind of thing that users would likely expect to work, even if we wouldn't necessarily encourage them to use it much.

jasnell · 2017-06-19T17:26:22Z

CI: https://ci.nodejs.org/job/node-test-pull-request/8734/

mscdex · 2017-06-19T17:49:21Z

Looks like this needs rebasing and lint errors fixed.

Add support for `Buffer.from(new String('...'))` and `Buffer.from({[Symbol.toPrimitive]() { return '...'; }})`

jasnell · 2017-06-19T18:01:07Z

Ugh. The most recent commit on master broke linting and was force pushed 2 hours after landing. It appears that it was force pushed as I was submitting this which broke everything. Rebasing now and will restart it

jasnell · 2017-06-19T18:02:23Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/8738/

Add support for `Buffer.from(new String('...'))` and `Buffer.from({[Symbol.toPrimitive]() { return '...'; }})` PR-URL: #13725 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

jasnell · 2017-06-19T22:31:12Z

Landed in 22cf25c

Add support for `Buffer.from(new String('...'))` and `Buffer.from({[Symbol.toPrimitive]() { return '...'; }})` PR-URL: #13725 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

gibfahn · 2018-01-15T22:01:31Z

Release team decided not to land on v6.x, if you disagree let us know.

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 16, 2017

nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Jun 16, 2017

mscdex reviewed Jun 16, 2017

View reviewed changes

BridgeAR reviewed Jun 16, 2017

View reviewed changes

mscdex reviewed Jun 16, 2017

View reviewed changes

addaleax approved these changes Jun 16, 2017

View reviewed changes

mscdex reviewed Jun 16, 2017

View reviewed changes

refack approved these changes Jun 16, 2017

View reviewed changes

refack reviewed Jun 16, 2017

View reviewed changes

jasnell force-pushed the buffer-from-boxed branch from 9baef7f to b38d925 Compare June 19, 2017 17:22

cjihrig approved these changes Jun 19, 2017

View reviewed changes

silverwind force-pushed the master branch from d3d66a5 to 24ecc33 Compare June 19, 2017 17:42

buffer: support boxed strings and toPrimitive

9b4c8b2

Add support for `Buffer.from(new String('...'))` and `Buffer.from({[Symbol.toPrimitive]() { return '...'; }})`

jasnell force-pushed the buffer-from-boxed branch from b38d925 to 9b4c8b2 Compare June 19, 2017 18:01

jasnell closed this Jun 19, 2017

addaleax mentioned this pull request Jun 21, 2017

v8.2.0 proposal #13744

Merged

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

ryzokuken mentioned this pull request May 21, 2018

Converting String-object to Buffer fails to copy data #13652

Closed

BridgeAR mentioned this pull request Sep 24, 2019

buffer: stricter buffer from #27051

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

buffer: support boxed strings and toPrimitive #13725

buffer: support boxed strings and toPrimitive #13725

jasnell commented Jun 16, 2017

mscdex Jun 16, 2017

mscdex Jun 16, 2017

jasnell Jun 16, 2017

BridgeAR Jun 16, 2017

jasnell Jun 16, 2017

mscdex Jun 16, 2017

addaleax Jun 16, 2017

mscdex Jun 16, 2017 •

edited

Loading

jasnell Jun 16, 2017

Fishrock123 commented Jun 16, 2017

refack Jun 16, 2017

jasnell Jun 17, 2017

refack Jun 17, 2017

refack Jun 17, 2017

jasnell Jun 17, 2017

refack Jun 16, 2017

jasnell commented Jun 17, 2017

jasnell commented Jun 19, 2017

mscdex commented Jun 19, 2017

jasnell commented Jun 19, 2017

jasnell commented Jun 19, 2017

jasnell commented Jun 19, 2017

gibfahn commented Jan 15, 2018

buffer: support boxed strings and toPrimitive #13725

buffer: support boxed strings and toPrimitive #13725

Conversation

jasnell commented Jun 16, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex Jun 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented Jun 17, 2017

jasnell commented Jun 19, 2017

mscdex commented Jun 19, 2017

jasnell commented Jun 19, 2017

jasnell commented Jun 19, 2017

jasnell commented Jun 19, 2017

gibfahn commented Jan 15, 2018

mscdex Jun 16, 2017 •

edited

Loading