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

Add buffer deprecation migration guide #1666

Merged
2 commits merged into from
May 30, 2018
Merged

Conversation

fhinkel
Copy link
Member

@fhinkel fhinkel commented May 29, 2018

@fhinkel fhinkel requested a review from Trott May 29, 2018 15:07

### Finding problematic bits of code using linters

Eslint rules [no-buffer-constructor](https://eslint.org/docs/rules/no-buffer-constructor)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: ESLint

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Done.

Note that `Buffer.alloc()` is also _faster_ on the current Node.js versions than
`new Buffer(size).fill(0)`, which is what you would otherwise need to ensure zero-filling.

Enabling eslint rule [no-buffer-constructor](https://eslint.org/docs/rules/no-buffer-constructor)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: ESLint

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Quick-read about-to-pack-for-a-flight LGTM!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

LGTM

Make sure that you do not use old `new Buffer` API — in any files where the line above is added,
using old `new Buffer()` API will _throw_. It will be easy to notice that in CI, though.

Alternatively, you could use [buffer-from](https://www.npmjs.com/package/buffer-from) and/or
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that does 4 additional dependencies is actually smaller in size than safer-buffer, maybe it could be fair to promote all three approaches more equally, and having drawbacks for each listed...

note: I'm the author of buffer-from, etc. so I'm probably very biased here 😄

Copy link
Member

Choose a reason for hiding this comment

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

@LinusU Would you be willing to draft up proposed alternate text for this part?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actual size of node_modules folder as reported by du -sh node_modules:

safer-buffer                              60K
buffer-{alloc,alloc-unsafe,fill,from}     48K

Copy link
Contributor

Choose a reason for hiding this comment

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

@Trott Sure thing!

Somebody should review it carefully so that I don't try to sell my own modules too much though 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

My proposal for the relevant part:

<a id="variant-2"></a>
## Variant 2: Use a polyfill

There are three different polyfills available:

- **[safer-buffer](https://www.npmjs.com/package/safer-buffer)** is a drop-in replacement for the
  entire `Buffer` API, that will _throw_ when using `new Buffer()`.

  You would take exactly the same steps as in [Variant 1](#variant-1), but with a polyfill
  `const Buffer = require('safer-buffer').Buffer` in all files where you use the new `Buffer` API.

  Make sure that you do not use old `new Buffer` API — in any files where the line above is added,
  using old `new Buffer()` API will _throw_. It will be easy to notice that in CI, though.

- **[buffer-from](https://www.npmjs.com/package/buffer-from) and/or
  [buffer-alloc](https://www.npmjs.com/package/buffer-alloc)** are
  [ponyfills](https://ponyfill.com/) for their respective part of the `Buffer` API. You only need
  to add the package(s) corresponding to the API you are using.

  You would import the module needed with an appropriate name, e.g.
  `const bufferFrom = require('buffer-from')` and then use that instead of the call to
  `new Buffer`, e.g. `new Buffer('test')` becomes `bufferFrom('test')`.

  A downside with this approach is slightly more code changes to migrate off them (as you would be
  using e.g. `Buffer.from` under a different name).

- **[safe-buffer](https://www.npmjs.com/package/safe-buffer)** is also a drop in replacement for
  the entire `Buffer` API, but using `new Buffer()` will still work as before.

  A downside to this approach is that it will allow you to also use the older `new Buffer()` API
  in your code, which is problematic since it can cause issues in your code, and will start
  emitting runtime deprecation warnings starting with Node.js 10
  ([read more here](https://github.com/chalker/safer-buffer#why-not-safe-buffer)).

Note that in either case, it is important that you also remove all calls to the old Buffer
API manually — just throwing in `safe-buffer` doesn't fix the problem by itself, it just provides
a polyfill for the new API. I have seen people doing that mistake.

Enabling ESLint rule [no-buffer-constructor](https://eslint.org/docs/rules/no-buffer-constructor)
or
[node/no-deprecated-api](https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-deprecated-api.md)
is recommended.

_Don't forget to drop the polyfill usage once you drop support for Node.js < 4.5.0._

Rendered:

screen shot 2018-05-30 at 01 04 55

Copy link
Contributor

Choose a reason for hiding this comment

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

ping @Trott @fhinkel ☝️

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mind if we merge this as is and put your changes in a separate PR to make reviewing and attribution easier?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sunds good 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Here it is :)

#1674

@ghost ghost merged commit adfe6cd into nodejs:master May 30, 2018
@vsemozhetbyt

This comment has been minimized.

@Trott
Copy link
Member

Trott commented May 31, 2018

Is this style and structure difference intended?

@vsemozhetbyt Unintentional, I'm sure. PR to fix welcome. @nodejs/website

fhinkel added a commit that referenced this pull request Jun 1, 2018
@fhinkel
Copy link
Member Author

fhinkel commented Jun 1, 2018

Not intended, thanks for noticing. I think I just fixed it.

@fhinkel fhinkel deleted the add-buffer-guide branch June 1, 2018 10:50
This pull request was closed.
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