Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

chore: Don't use the deprecated Buffer constructor #979

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

realityking
Copy link
Contributor

🚀 Why this change?

The Buffer constructor is deprecated (it will likely start throwing warnings in Node.js 10), instead Buffer.{from|alloc|allocUnsafe} should be used.

Discussion of the API change: https://medium.com/@jasnell/node-js-buffer-api-changes-3c21f1048f97
Runtime deprecation planning: nodejs/node#19079

📝 Related issues and Pull Requests

This is also preparation for #975.

✅ What didn't I forget?

  • To write docs
  • To write tests
  • To put Conventional Changelog prefixes in front of all my commits and run npm run lint

@ChALkeR
Copy link

ChALkeR commented Mar 2, 2018

Uh-oh. Without this patch, dredd apparently sends a chunk of uninitialized memory as auth headers when configuration.options.user is a number (it's read from a yml config and is typed afaik). Doesn't seem to be serious due to use-cases, but still not a nice thing.

@realityking
Copy link
Contributor Author

@ChALkeR Only if configuration.options.user is a number. I assume it's a string so it'll be this behaviour: https://nodejs.org/api/buffer.html#buffer_new_buffer_string_encoding

@realityking
Copy link
Contributor Author

@honzajavorek The tests on Node 8 failed. Could you restart that job?

@honzajavorek
Copy link
Contributor

@realityking Just did :)

@honzajavorek
Copy link
Contributor

if configuration.options.user is a number

Can we just always coerce it to a string in the configuration?

@realityking
Copy link
Contributor Author

Can we just always coerce it to a string in the configuration?

That'd be possible. Behaviour with the patch as it is would be to throw the error TypeError: "value" argument must not be a number.

Copy link
Contributor

@honzajavorek honzajavorek left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you! ✨

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants