Skip to content

buffer: do not emit deprecation notice on Buffer.of #19682

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

Closed
TimothyGu wants to merge 3 commits into from
Closed

buffer: do not emit deprecation notice on Buffer.of #19682

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0eaff92
buffer: do not emit deprecation notice on Buffer.of
TimothyGu Mar 29, 2018
9491d3b
fixup! buffer: do not emit deprecation notice on Buffer.of
TimothyGu Mar 29, 2018
4be9962
fixup! buffer: do not emit deprecation notice on Buffer.of
TimothyGu Mar 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
buffer: do not emit deprecation notice on Buffer.of
  • Loading branch information
@TimothyGu
TimothyGu committed Mar 29, 2018
commit 0eaff92eba7b4b92f47161b208bb199c18222a51
11 changes: 11 additions & 0 deletions lib/buffer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,17 @@ Buffer.from = function from(value, encodingOrOffset, length) {
);
};

// Identical to the built-in %TypedArray%.of(), but avoids using the deprecated
// Buffer() constructor.
//
// Refs: https://tc39.github.io/ecma262/#sec-%typedarray%.of
Buffer.of = (...items) => {
Copy link
Member

@ChALkeR ChALkeR Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This changes the name of the function. It should probably be named 'of'.

Copy link
Member Author

@TimothyGu TimothyGu Mar 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Grrr, I always thought this kind of assignment names the function automatically. Fixed.

Copy link
Member

@ChALkeR ChALkeR Mar 30, 2018

Choose a reason for hiding this comment

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

They are named in cases like { of: () => {} } and const of = () => {}, but not in x.of = () => {}.

const newObj = allocate(items.length);
Copy link
Member

@addaleax addaleax Mar 29, 2018

Choose a reason for hiding this comment

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

Hm … we may want new FastBuffer(items.length) here, so that the returned buffer is not pooled, like you’d expect from the standard TypedArray methods.

Also, I’m curious, do you know why TypedArray.of does not use this[Symbol.species]? That seems like a good fix for this problem in the long run.

Copy link
Member Author

@TimothyGu TimothyGu Mar 29, 2018

Choose a reason for hiding this comment

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

Hmm, good point. I'll opt for createUnsafeBuffer() then, as we don't need the buffer to be zero-filled.

Good question. Let me ask: tc39/ecma262#1157

Copy link
Member

@ChALkeR ChALkeR Mar 29, 2018

Choose a reason for hiding this comment

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

Is it different from Buffer.from(items)?

Copy link
Member Author

@TimothyGu TimothyGu Mar 29, 2018

Choose a reason for hiding this comment

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

Yes; Buffer.of is (...items).

Copy link
Member

@ChALkeR ChALkeR Mar 29, 2018

Choose a reason for hiding this comment

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

That's already in the header, but is the implementation here different in behaviour from just returning Buffer.from(items)? Do we need to reimplement it?

for (var k = 0; k < items.length; k++)
newObj[k] = items[k];
return newObj;
Copy link
Member

@ChALkeR ChALkeR Mar 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Line-to-line code duplication of fromArrayLike.
These four lines could be replaced with return fromArrayLike(items).

Copy link
Member Author

@TimothyGu TimothyGu Mar 30, 2018

Choose a reason for hiding this comment

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

No. fromArrayLike uses allocate() while this function uses createUnsafeBuffer(). The former may be pooled, which is not desirable.

};

Object.setPrototypeOf(Buffer, Uint8Array);

// The 'assertSize' method will remove itself from the callstack when an error
Expand Down
8 changes: 8 additions & 0 deletions test/parallel/test-buffer-of-no-deprecation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
// Flags: --pending-deprecation --no-warnings
'use strict';

const common = require('../common');

process.on('warning', common.mustNotCall());
Copy link
Contributor

@cjihrig cjihrig Mar 30, 2018

Choose a reason for hiding this comment

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

Should the --no-warnings flag be dropped?

Copy link
Member

@jasnell jasnell Apr 1, 2018

Choose a reason for hiding this comment

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

No, the --no-warnings just stops the console output. The process.on('warning') event will still emit.


Buffer.of(0, 1);