Use req.socket instead of req.connection #8

[briceruzand]

Fixes #8

I used fastify and jshttp/forwarded generate a warning due to that deprecation usage.

Related fastify issues :

• Deprecated API usage (#2575) fastify/fastify#2594
• fix: add missing request.socket (#87) fastify/light-my-request#99

[al-bglhk]

Thank you for the fix! I am hitting the same warning from the logs. When will this fix be released?

[mcollina]

FIY: req.connection is deprecated in Node.js core. https://nodejs.org/api/http.html#http_request_connection
req.socket has been there since 0.3

[dougwilson]

Thanks for this PR, and sorry I was not subscribed to this module's notifications.

I see no issue with reading the new property, but I am a little afraid about modules on npm that are trying to overwrite the ip or mock http may end up having an issue once this is merged.

I am thinking about the impact to the use in Express, so it may extend to other things using this.

So I'm wondering if it would be possible to keep reading both values or something. I will try to research into what the mocks and such are doing at least in the Express ecosystem to assess the impact of switching to a new property, but anyone is welcome to post an assessment as well.

No assessment on the impact has been provided so far, so I will try to do it this weekend since Node.js has deprecated the property used here (though I'm not sure what the impact of using the old property is).

[dougwilson]

Just as an update before I go to sleep, I looked around a bit, and there are definately a fair number of mock libraries on public npm that only provide a req.socket and no req.connection. Of course, as well as a lot of test suites on public github not using a mock library. Here is a specific mock module that does not seem compatible with this change: https://www.npmjs.com/package/node-mocks-http (note: this is not comprehensive, as I haven't looked for very long).

So ideally if there is a way to make this change backwards-compatible that would be amazing.

If it is not possible, then we may want to figure out what our next steps should be.

Anyway, the above can still be deferred until there is a more comprehensive list of affected mock libraries, etc. as well.

[dougwilson]

Sorry, I just realized before I signed off, that I... swapped req.socket and req.connection in my mind :( I was looking for the wrong property, haha. So I guess I didn't actually even start on looking, doh! I will look tomorrow for the correct property...

[mcollina]

FYI - proxy-addr tests only mock .connection. I think we should release this as patch, so the best option is to support both.

[mcollina]

Suggested change

	var socketAddr = req.socket.remoteAddress
	var socketAddr = req.socket ? req.socket.remoteAddress : req.connection.remoteAddress

A test should be added to cover this case.

[dougwilson]

Hi @briceruzand I went ahead and incorporated all the changes from this PR as well as added tests for these changes since I didn't hear any objection about the changes, but also the changes were never accepted in to the PR.