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

FIX: update sftp after changes in latest nodejs constants #47

Closed
wants to merge 1 commit into from
Closed

FIX: update sftp after changes in latest nodejs constants #47

wants to merge 1 commit into from

Conversation

wdavidw
Copy link
Contributor

@wdavidw wdavidw commented Sep 1, 2016

Reference to Node.js constants are broken since Node.js version 6.3.0 after #6534. Here's an illustration of the problem:

# Node v6.2.0
[ `node -e "process.binding('constants').S_IFMT" -p` -eq '61440' ]
# Node v6.3.0
[ `node -e "process.binding('constants').fs.S_IFMT" -p` -eq '61440' ]

The proposed pull request fix it in a backward compatible way.

@mscdex
Copy link
Owner

mscdex commented Sep 1, 2016

I think an easier way might be to just change the constants declaration at the top like:

var constants = require('fs').constants || process.binding('constants');

@wdavidw
Copy link
Contributor Author

wdavidw commented Sep 1, 2016

Agreed, only "constants" disturbs me in term of semantic, are you ok with "constants_fs = ..."

@mscdex
Copy link
Owner

mscdex commented Sep 1, 2016

I think keeping the name is fine, it's only ever fs constants (that ssh2-streams uses) anyway so there really should be no confusion.

@wdavidw
Copy link
Contributor Author

wdavidw commented Sep 2, 2016

Fine, should be ready now

@wdavidw
Copy link
Contributor Author

wdavidw commented Sep 6, 2016

Could you please accept this request if everything is ok with u, we need this fix. Thanks

@mscdex
Copy link
Owner

mscdex commented Sep 8, 2016

Landed in c0b0f76. Thanks!

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.

2 participants