Skip to content

avoid process.binding() where possible - #5

Merged
ruyadorno merged 5 commits into
isaacs:masterfrom
Trott:no-binding
Jan 20, 2020
Merged

ruyadorno merged 5 commits into
isaacs:masterfrom
Trott:no-binding

Conversation

@Trott

@Trott Trott commented Oct 19, 2019

Copy link
Copy Markdown
Contributor

process.binding() is deprecated. When running recent versions of Node.js
with --pending-deprecation, the current code results in a printed
warning about process.binding() usage. process.binding() is used to
create writev(), but Node.js now exposes fs.writev(). Use that instead
when it exists. Otherwise, fall back to the polyfill. This will avoid
runtime deprecation messages for end users.

If the minimum supported engine is ever Node.js 12.9.0 or above, the
polyfill code should be removable at that time.

Trott added 2 commits October 19, 2019 16:34
process.binding() is deprecated. When running recent versions of Node.js
with --pending-deprecation, the current code results in a printed
warning about process.binding() usage. process.binding() is used to
create writev(), but Node.js now exposes fs.writev(). Use that instead
when it exists. Otherwise, fall back to the polyfill. This will avoid
runtime deprecation messages for end users.

If the minimum supported engine is ever Node.js 12.9.0 or above, the
polyfill code should be removable at that time.

@mikemimik mikemimik left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM.

Two comments.

Comment thread index.js Outdated
Comment thread index.js Outdated
Comment thread index.js Outdated
@ruyadorno

Copy link
Copy Markdown
Contributor

@Trott are these changes node@6 compatible? otherwise we'll have to wait for npm@7

@Trott

Trott commented Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

@Trott are these changes node@6 compatible? otherwise we'll have to wait for npm@7

I believe this will support all versions of Node.js that the current version supports. It uses the new feature if it exists, otherwise it falls back to the previous code.

@Trott

Trott commented Jan 14, 2020

Copy link
Copy Markdown
Contributor Author

(Pushed another commit to add a comment indicating that the polyfill block is needed for Node.js earlier than 12.9.0 so that it can be removed in a few years if support for earlier versions is no longer needed.)

@ruyadorno
ruyadorno dismissed mikemimik’s stale review January 20, 2020 18:13

All comments adressed

@ruyadorno
ruyadorno merged commit e0b51ea into isaacs:master Jan 20, 2020
@ruyadorno

Copy link
Copy Markdown
Contributor

@Trott landed in v2.0.1 🎉

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.

4 participants