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

Breaking: deprecate safe-buffer.Buffer calls #110

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Mar 2, 2018

safe-buffer.Buffer function/constructor is just a re-export of buffer.Buffer and should be deprecated likewise.

Otherwise, using Buffer = require('safe-buffer').Buffer magically hides the deprecation message, which it shouldn't, as that code will actually cause runtime deprecation errors on Node.js >= 10.

In fact, ecosystem analysis shows that people do that a lot to silence standard linter — they see a recommendation to use safe-buffer in the warning, they throw in var Buffer = require('buffer').Buffer, the warning goes away, nothing actually changes.
Example:

Also, even if they migrate to the new API and install safe-buffer for the actual polyfills, the fact that that line alone disables the deprecation message makes them overlook calls to the old API.
Examples:

Old Buffer API is going to be runtime-deprecated in Node.js 10.0 (nodejs/node#19079) and require('safe-buffer').Buffer(arg) actually hits that deprecated API, so this change is needed for developers to spot those issues that would soon become runtime warnings, in advance. eslint-plugin-node was already helping developers to spot that deprecated Buffer API usage in advance, but only until they put Buffer = require('safe-buffer').Buffer into their code (and they do that a lot, since that's the recommended way to keep older Node.js versions support).

Actually, I am not even sure if this should be a breaking or a bugfix change. Thoughts?

/cc @Trott, @feross .

safe-buffer.Buffer function/constructror is just a re-export of
buffer.Buffer and should be deprecated likewise.

Otherwise, using Buffer = require('safe-buffer').Buffer magically hides
the deprecation message, which it shouldn't, as that code will
actually cause runtime deprecation errors on Node.js >= 10.
@codecov-io
Copy link

Codecov Report

Merging #110 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #110      +/-   ##
==========================================
+ Coverage   98.21%   98.21%   +<.01%     
==========================================
  Files          43       43              
  Lines        1065     1066       +1     
==========================================
+ Hits         1046     1047       +1     
  Misses         19       19
Impacted Files Coverage Δ
lib/util/deprecated-apis.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5260039...721251c. Read the comment docs.

@mysticatea
Copy link
Owner

Sounds good to me. Thank you!

@mysticatea mysticatea merged commit 2dbf741 into mysticatea:master Mar 9, 2018
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.

None yet

3 participants