Skip to content

Use Buffer.(from|alloc) instead of deprecated Buffer API#46

Closed
ChALkeR wants to merge 1 commit into
TritonDataCenter:masterfrom
ChALkeR:buffer-fix
Closed

Use Buffer.(from|alloc) instead of deprecated Buffer API#46
ChALkeR wants to merge 1 commit into
TritonDataCenter:masterfrom
ChALkeR:buffer-fix

Conversation

@ChALkeR

@ChALkeR ChALkeR commented Mar 21, 2018

Copy link
Copy Markdown
Contributor

This also includes a dependnecy on a polyfill targeting older Node.js versions where Buffer.alloc() and Buffer.from() API is not implemented (Node.js < 4.5.0 and some 5.x versions).

This is mostly a line-to-line change, though in some cases, minor code simplification was done, e.g.

var version = new Buffer(1);
version.writeUInt8(1, 0);

was transformed into

var version = Buffer.from([1]);

Ref: nodejs/node#19079
Ref: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor
Ref: https://nodejs.org/api/buffer.html#buffer_class_buffer
Ref: https://github.com/ChALkeR/safer-buffer/blob/master/Porting-Buffer.md
Fixes: #45

Note that this does not completely solve the problem yet, for a complete solution, asn1 (a dependency) should also receive this fix (see below for that one).

But fixing the API usage in this package is still required, and a significant portion of tests pass without ever hitting the deprecated Buffer API in asn1 dependency.

This also includes a dependnecy on a polyfill targeting older Node.js
versions where Buffer.alloc() and Buffer.from() API is not implemented
(Node.js < 4.5.0 and some 5.x versions).

Ref: nodejs/node#19079
Ref: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor
Ref: https://nodejs.org/api/buffer.html#buffer_class_buffer
Ref: https://github.com/ChALkeR/safer-buffer/blob/master/Porting-Buffer.md
@ChALkeR

ChALkeR commented Mar 21, 2018

Copy link
Copy Markdown
Contributor Author

asn1 fix in TritonDataCenter/node-asn1#30.

@BYK

BYK commented May 25, 2018

Copy link
Copy Markdown

@arekinath would be great if you can merge this and release a new version as this is blocking Yarn from being Node 10 compatible.

We can use the fork but that's less than ideal.

@ChALkeR

ChALkeR commented May 31, 2018

Copy link
Copy Markdown
Contributor Author

/cc @cjihrig and @Trott perhaps?

@stevestock

Copy link
Copy Markdown

@arekinath

@shahkashani

Copy link
Copy Markdown

Bump <3 Would love to get this (and yarnpkg/yarn#5477) fixed!

joyent-automation pushed a commit that referenced this pull request Jun 5, 2018
Reviewed by: Alex Wilson <alex.wilson@joyent.com>
Reviewed by: Cody Peter Mello <cody.mello@joyent.com>
@arekinath

Copy link
Copy Markdown

Merged as 175758a, released in 1.14.2

@arekinath arekinath closed this Jun 5, 2018
@ChALkeR

ChALkeR commented Jun 5, 2018

Copy link
Copy Markdown
Contributor Author

@arekinath Thanks!

@BYK

BYK commented Jun 7, 2018

Copy link
Copy Markdown

@arekinath thanks a lot for the merge and release! It would be ideal if we can propagate this fix all the way to upstream. There are a few more fixes on joyent repos:

Do you think you can help with those too?

And since sshpk depends on asn1, we'd need to make another patch and release here once that PR is merged and released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Buffer constructor runtime deprecation - this package emits a warning on Node 10

5 participants