Skip to content

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