buffer: support boxed strings and toPrimitive by jasnell · Pull Request #13725 · nodejs/node

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

== null ?

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

lib/buffer.js

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

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

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

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

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

lib/buffer.js

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

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

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <anna@addaleax.net> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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

lib/buffer.js Outdated

Copy link

Contributor

mscdex Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

== null ?

jasnell reacted with thumbs up emoji

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

lib/buffer.js Outdated

Copy link

Member

addaleax Jun 16, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Uh oh!

Comments

Conversation

jasnell commented Jun 16, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex Jun 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Jun 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 17, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

mscdex commented Jun 19, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

mscdex Jun 16, 2017 •

edited

Loading