GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — buffer: make byteLength work with ArrayBuffer & DataView by JacksonTian · Pull Request #5255 · nodejs/node

Conversation

@JacksonTian

@JacksonTian JacksonTian commented Feb 16, 2016

Copy link
Copy Markdown
Contributor

Convert anything to string, but Buffer, TypedArray and ArrayBuffer

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Feb 16, 2016
@bnoordhuis

bnoordhuis commented Feb 16, 2016

Copy link
Copy Markdown
Member

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/1676/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ChALkeR

ChALkeR commented Feb 16, 2016

Copy link
Copy Markdown
Member

Is this semver-minor or semver-major?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Feb 16, 2016

Copy link
Copy Markdown
Member

I'd say semver-minor
/cc @trevnorris
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 16, 2016
@bnoordhuis

bnoordhuis commented Feb 16, 2016

Copy link
Copy Markdown
Member

I'd say semver-minor as well, it's additive. Running the CI one more time to see if the ARM failures are transient: https://ci.nodejs.org/job/node-test-pull-request/1677/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ChALkeR

ChALkeR commented Feb 16, 2016

Copy link
Copy Markdown
Member

Well, it actually changes the result on certain input values, but I guess that it would be safe to assume that noone is relying on the old behavior.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@trevnorris

trevnorris commented Feb 16, 2016

Copy link
Copy Markdown
Contributor

docs need to be updated with newly accepted arguments. implementation LGTM.

Mind making a comment in the commit message body about why this functionality is useful?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 21, 2016

Copy link
Copy Markdown
Member

@JacksonTian ... ping...
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JacksonTian
JacksonTian force-pushed the byteLength branch from 93fcae9 to a5c5d70 Compare March 22, 2016 03:48
@JacksonTian

JacksonTian commented Mar 22, 2016

Copy link
Copy Markdown
Contributor Author

Hi, @trevnorris @jasnell , I updated it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell
jasnell reviewed Mar 22, 2016
View reviewed changes
Comment thread doc/api/buffer.markdown Outdated

@jasnell jasnell Mar 22, 2016

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

nit: {String | Buffer | DataView | ArrayBuffer}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@trevnorris trevnorris Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This doesn't explicitly mention the encompasing of all typed arrays. there a way that can be included?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@trevnorris trevnorris Mar 23, 2016

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I ask this b/c everything listed here is a discrete constructor. Which at quick glance would show that all typed arrays are not supported.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

LGTM with some nits
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Mar 22, 2016

Copy link
Copy Markdown
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/2011/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@JacksonTian
buffer: make byteLength work with ArrayBuffer & DataView
0c74f60 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```
@JacksonTian
JacksonTian force-pushed the byteLength branch from a5c5d70 to 0c74f60 Compare March 23, 2016 14:09
@JacksonTian

JacksonTian commented Mar 23, 2016

Copy link
Copy Markdown
Contributor Author

Hi @trevnorris , I added the TypedArray links.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Mar 27, 2016
@JacksonTian @jasnell
buffer: make byteLength work with ArrayBuffer & DataView
293fd04 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```

PR-URL: #5255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell

jasnell commented Mar 27, 2016

Copy link
Copy Markdown
Member

Landed in 293fd04
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Mar 27, 2016
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
@JacksonTian @evanlucas
buffer: make byteLength work with ArrayBuffer & DataView
ee83c95 
Convert anything to string, but Buffer, TypedArray and ArrayBuffer

```
var uint8 = new Uint8Array([0xf0, 0x9f, 0x90]);
Buffer.byteLength(uint8); // should be 3, but returns 11
Buffer.byteLength(uint8.buffer); // should be 3, but return 20
```

PR-URL: #5255
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
9cd6e98 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. (Forrest L Norvell)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Mar 31, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
1bb2c4b 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
79ea8c3 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)
evanlucas added a commit that referenced this pull request Apr 1, 2016
@evanlucas
2016-03-31 Version 5.10.0 (Stable) Release
64c87e2 
Notable changes:

* buffer:
  * make byteLength work with ArrayBuffer & DataView (Jackson Tian)
[#5255](#5255)
  * backport --zero-fill-buffers command line option (James M Snell)
[#5744](#5744)
  * backport new buffer constructor APIs (James M Snell)
[#5763](#5763)
  * add swap16() and swap32() methods (James M Snell)
[#5724](#5724)
* fs: add the fs.mkdtemp() function. (Florian MARGAINE)
[#5333](#5333)
* net: emit host in lookup event (HUANG Wei)
[#5598](#5598)
* node: --no-browser-globals configure flag (Fedor Indutny)
[#5853](#5853)
* npm: Upgrade to v3.8.3. Fixes a security flaw in the use of
authentication tokens in HTTP requests that would allow an attacker to
set up a server that could collect tokens from users of the command-line
interface. Authentication tokens have previously been sent with every
request made by the CLI for logged-in users, regardless of the destination
of the request. This update fixes this by only including those tokens
for requests made against the registry or registries used for the
current install. (Forrest L Norvell)
[npm#6](npm#6)
* repl: support standalone blocks (Prince J Wesley)
[#5581](#5581)
* src: override v8 thread defaults using cli options (Tom Gallacher)
[#4344](#4344)

PR-URL: #5970
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

buffer Issues and PRs related to the buffer subsystem. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@JacksonTian @bnoordhuis @ChALkeR @jasnell @trevnorris @mscdex
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.