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.

Sign up for GitHub

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

Jump to bottom

Add Buffer#includes() #3552

Closed
sindresorhus opened this issue Oct 27, 2015 · 7 comments
Closed

Add Buffer#includes() #3552

sindresorhus opened this issue Oct 27, 2015 · 7 comments
Labels
buffer Issues and PRs related to the buffer subsystem. feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. v8 engine Issues and PRs related to the V8 dependency.

Comments

@sindresorhus
Copy link

For convenience, code clarity, and parity with TypedArray. See TypedArray#includes().

The includes() method determines whether a typed array includes a certain element, returning true or false as appropriate.
@evanlucas evanlucas added the feature request Issues that request new features to be added to Node.js. label Oct 27, 2015
@targos
Copy link
Member

targos commented Oct 27, 2015

TypedArray#includes and Array#includes are not shipped in V8 yet.
When it happens, buffers will have the method. You can test it with the harmony flag:

% node -v
v5.0.0-rc.1
% node --harmony
> var buffer = new Buffer(1).fill(1)
undefined
> buffer.includes(1)
true

@mscdex mscdex added buffer Issues and PRs related to the buffer subsystem. v8 engine Issues and PRs related to the V8 dependency. labels Oct 27, 2015
@trevnorris
Copy link
Contributor

Though we might want to override it. v8's implementation (following the spec) is restrictive and slow. e.g. indexOf(), fill(). Instead, to have feature parity with current Buffer API's we should consider our own impl that accepts multi-char strings, etc.

@targos
Copy link
Member

targos commented Oct 28, 2015

@trevnorris good point! Then we can just wrap it around indexOf.

@trevnorris
Copy link
Contributor

Oh yeah. Good call. Easy PR for the taking.

@sindresorhus
Copy link
Author

I made a polyfill: https://github.com/sindresorhus/buffer-includes

@cjihrig cjihrig added the good first issue Issues that are suitable for first-time contributors. label Oct 28, 2015
@umamialex umamialex mentioned this issue Oct 28, 2015
Closed
@rodmachen rodmachen mentioned this issue Oct 28, 2015
Closed
@rodmachen
Copy link
Contributor

Just added a PR to implement this.

#3568

@Fishrock123
Copy link
Contributor

PR is #3567

cjihrig pushed a commit that referenced this issue Dec 16, 2015
@umamialex @cjihrig
buffer: add includes() for parity with TypedArray
9ab669f 
Add Buffer#includes() by wrapping an indexOf and performing a strict
equals check to -1.

The includes method takes the search value, byteOffset, and encoding as
arguments.

The test is a modified version of the indexOf test.

Fixes: #3552
PR-URL: #3567
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
@umamialex
buffer: add includes() for parity with TypedArray
ff8a634 
Add Buffer#includes() by wrapping an indexOf and performing a strict
equals check to -1.

The includes method takes the search value, byteOffset, and encoding as
arguments.

The test is a modified version of the indexOf test.

Fixes: nodejs#3552
PR-URL: nodejs#3567
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
buffer Issues and PRs related to the buffer subsystem. feature request Issues that request new features to be added to Node.js. good first issue Issues that are suitable for first-time contributors. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@mscdex @sindresorhus @trevnorris @evanlucas @Fishrock123 @targos @cjihrig @rodmachen