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

avoid process.binding() where possible #5

Merged
merged 5 commits into from
Jan 20, 2020
Merged

Conversation

Trott
Copy link
Contributor

@Trott Trott commented Oct 19, 2019

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.

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.
Copy link

@mikemimik mikemimik left a comment

Choose a reason for hiding this comment

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

LGTM.

Two comments.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
@ruyadorno
Copy link
Contributor

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

@Trott
Copy link
Contributor Author

Trott commented Jan 14, 2020

@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
Copy link
Contributor Author

Trott commented Jan 14, 2020

(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
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