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 compatibility with Node.js master #312

Merged
merged 1 commit into from
May 23, 2017

Conversation

lpinca
Copy link
Contributor

@lpinca lpinca commented Apr 12, 2017

Like #309 but takes into account Node.js versions below 4.

@@ -189,6 +189,8 @@ proto._onStream = function _onStream (stream) {
socket = new net.Socket(socketOptions)
}

if (process.versions.modules >= 46) { socket.server = this }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently this is only needed for Node.js master but also handles the unlikely case where nodejs/node#11926 will be backported to Node.js 6 and 4 (modules >= 46).

Copy link
Contributor

@ronkorving ronkorving Apr 13, 2017

Choose a reason for hiding this comment

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

This kind of code would be well served with a code comment :) Both the if-condition as well as the server assignment are puzzling to someone who doesn't know the background behind this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed will add in a bit.

@lpinca
Copy link
Contributor Author

lpinca commented Apr 17, 2017

@diasdavid can you please take a look when you have time?
nodejs/node#11926 has not yet landed in a v7 release as it breaks this module.
It would be great to know if this fix/workaround is acceptable.

@daviddias daviddias merged commit 81edd1a into spdy-http2:master May 23, 2017
@lpinca lpinca deleted the support/nodejs-8 branch May 23, 2017 05:32
@refack
Copy link

refack commented May 23, 2017

Compatibility restoration confirmed
Ref: https://github.com/nodejs/node/wiki/CITGM-Status

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants