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.

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

Use Buffer.from instead of 'new Buffer' #175

Closed
wants to merge 1 commit into from

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Feb 22, 2018

Buffer() constructor is deprecated.

Refs: https://nodejs.org/api/deprecations.html#deprecations_dep0005_buffer_constructor

Note: this does not cover tests/benchmarks.

Supported Node.js range is not affected, Buffer.alloc is used already and the minimum required Node.js version is 4.5.0.

This is one of the three changes needed to keep npm i working under NODE_PENDING_DEPRECATION=1 without warnings. The other two are mafintosh/flush-write-stream#3 and mafintosh/duplexify#17.

Copy link

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM. Also, there should be no compatibility concerns, given that node-tar already uses Buffer.alloc() extensively.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 2, 2018

Tracking: nodejs/node#19079

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

We can't do this unless it uses safe-buffer as well, because this breaks in node@>4 <4.5. If you can add that dep and put const Buffer = require('safe-buffer').Buffer, that'd be handy!

@shinnn
Copy link

shinnn commented Mar 8, 2018

because this breaks in node@>4 <4.5

As @ChALkeR said, this library already drops support for Node.js < 4.5.

https://github.com/npm/node-tar/blob/a561d633e219d912fcf61606566fa2c6e35332fb/package.json#L38

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 8, 2018

@zkat Hi! I think you made a mistake here — this package doesn't work on Node.js < 4.5.0 and doesn't declare support for it. I already mentioned that in my PR.

  1. It already uses unpolyfilled Buffer.alloc not only in tests but, since 03dbfa7 — in lib. That's since node-tar@3.0.0 release, and the current one is node-tar@4.4.0.
  2. Since d431b83 (require node 4.5 as minimum #121), that's node-tar@3.1.10 the minimum declared supported Node.js version is 4.5.0.

@isaacs merged both of those separately, so I suppose that's deliberate.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 13, 2018

@zkat Ping?

@zkat
Copy link
Contributor

zkat commented Mar 14, 2018

Ah, I didn't realize. /cc @isaacs 'cause we should support this, and it's fairly trivial to have Buffer-related support for node@4 even under 4.5. Until npm actually releases npm@6, I would consider this a bug in node-tar?

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 20, 2018

@zkat Does this PR need to be blocked on that?
That is a separate issue, and this PR does not change Node.js version range support.

Copy link
Contributor

@zkat zkat left a comment

Choose a reason for hiding this comment

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

I guess not. I thought we'd changed things so npm ran in pre-4.5 node, but that doesn't seem to be the case. And we're dropping node@4 support now with npm@6 anyway.

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 20, 2018

@zkat Thanks! 👍

isaacs added a commit that referenced this pull request Mar 20, 2018
@isaacs isaacs closed this in 4513e3b Mar 20, 2018
@isaacs
Copy link
Owner

isaacs commented Mar 20, 2018

Updated node-tar so this is no longer an issue. Should work fine in every node >=4.0.0.

It's weird that we were apparently already not working in Node 4.x <4.5.0 for a while, and hadn't heard about it. I guess people tend to stick to the tip of their major branch?

@ChALkeR
Copy link
Contributor Author

ChALkeR commented Mar 20, 2018

@isaacs Thanks! I have not included the tests because I am mass-filing these PRs, and users won't observe the warnings from tests =).

Per nodejs/node#19079 (comment) I am also now sending lists of per-owner affected packages via Gitter.

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.

5 participants