fix: add missing request.socket (#87) #99

humphd · 2020-09-24T20:17:44Z

Fixes #87. This adds the .socket property to request, mirroring the .connection property

Checklist

run npm run test and npm run benchmark
tests and/or benchmarks are included
documentation is changed or added
commit message and code follows the Developer's Certification of Origin
and the Code of conduct

Eomm

Could you add a test case?

jsumners · 2020-09-24T21:35:02Z

lib/request.js

  this.connection = {
    remoteAddress: options.remoteAddress || '127.0.0.1'
  }
+  this.socket = this.connection


Should there be some sort of deprecation here? We'd have to follow the upstream deprecation cycle.

I think you can add the deprecate warning in the typing to something like this

interface Request extends stream.Readable { ... // @deprecated connection: object ...

I agree with @jsumners.
According to the Node.js docs, .connection has been deprecated in favor of .socket.
I think we should do something like this:

this.socket = { remoteAddress: options.remoteAddress || '127.0.0.1' } Object.defineProperty(Request, 'connection', { get () { // if node major ≥13 emit a warning with https://github.com/fastify/fastify-warning return this.socket } })

humphd · 2020-09-25T19:17:04Z

Thanks for all the feedback. I've tried to make the changes/updates that were requested. Let me know if I've done it incorrectly, or there are other things I should consider.

jsumners · 2020-09-25T19:25:34Z

lib/request.js

+
+  Object.defineProperty(this, 'connection', {
+    get () {
+      // if node major ≥13 emit a warning with https://github.com/fastify/fastify-warning


The comment here is not sufficient. An actual test against against process.version is needed along with a warning issued through the interface that fastify-warning provides.

Understood. Maybe something like https://github.com/fastify/fastify-plugin/blob/d3b2ca47404e622f7c474493bb0624bf95a2e4f4/test/esm/index.test.js#L6, and then use fastify-warning to actually log that message?

Correct. Fastify itself uses fastify-warning to issue deprecation notices. You can learn from there as well.

Great, thanks @jsumners, this is really helpful. I'll dig around and update this to do it correctly.

mcollina · 2020-10-01T09:12:26Z

Any update on this @humphd ?

humphd · 2020-10-01T17:17:49Z

I've submitted another fix, which uses semver to do the check against the node version, and fastify-warning to emit the deprecation warning. Thanks to @jsumners for pointing out how I should do this.

A couple things I wasn't sure about related to the way I named things in warning.create(), specifically the CODE and if there is a standard you want to use. Also, I noticed that in some places you create all the warnings in a single file. I did it in the request.js file, but could move it.

Also, I ran into a small issue with naming on the code, where I had lowercase letters for the plugin name, but everything gets converted to uppercase. I'll send a PR to the docs for fastify-warning to make that more clear.

mcollina · 2020-10-01T17:32:07Z

lib/request.js


+  Object.defineProperty(this, 'connection', {
+    get () {
+      if (semver.gte(process.versions.node, '13.0.0')) {


let's just emit the deprecation, I prefer we do not add another dependency here.

Sounds good to me.

humphd · 2020-10-01T18:09:29Z

Updated to remove semver and do the warning on .connection regardless of version.

jsumners

LGTM

mcollina

lgtm

Before this change, if a Fastify user attached an `onTimeout` hook to their server, an error would be thrown in test mode when injecting requests with light-my-request ``` TypeError: request.raw.socket.on is not a function at Object.routeHandler [as handler] (../../node_modules/fastify/lib/route.js:350:28) at Router.lookup (../../node_modules/find-my-way/index.js:356:14) at ../../node_modules/light-my-request/index.js:101:38 at Request.prepare (../../node_modules/light-my-request/lib/request.js:110:12) at ../../node_modules/light-my-request/index.js:101:11 at doInject (../../node_modules/light-my-request/index.js:97:12) at Chain.<computed> [as then] (../../node_modules/light-my-request/index.js:187:23) at runMicrotasks (<anonymous>) ``` This fixes the socket object to have an `on` function (as well as the rest of the EventEmitter interface) so that things that add timeout handlers can at least add them without erroring. This doesn't implement actual timeout injection, but I think that could be done in userland by emitting events on the socket manually. Similar to fastify#99.

…#101) Before this change, if a Fastify user attached an `onTimeout` hook to their server, an error would be thrown in test mode when injecting requests with light-my-request ``` TypeError: request.raw.socket.on is not a function at Object.routeHandler [as handler] (../../node_modules/fastify/lib/route.js:350:28) at Router.lookup (../../node_modules/find-my-way/index.js:356:14) at ../../node_modules/light-my-request/index.js:101:38 at Request.prepare (../../node_modules/light-my-request/lib/request.js:110:12) at ../../node_modules/light-my-request/index.js:101:11 at doInject (../../node_modules/light-my-request/index.js:97:12) at Chain.<computed> [as then] (../../node_modules/light-my-request/index.js:187:23) at runMicrotasks (<anonymous>) ``` This fixes the socket object to have an `on` function (as well as the rest of the EventEmitter interface) so that things that add timeout handlers can at least add them without erroring. This doesn't implement actual timeout injection, but I think that could be done in userland by emitting events on the socket manually. Similar to #99.

fix: add missing request.socket (fastify#87)

3cc3a5d

Eomm reviewed Sep 24, 2020

View reviewed changes

jsumners reviewed Sep 24, 2020

View reviewed changes

Add test, update typing, add connection property

2ada0be

jsumners reviewed Sep 25, 2020

View reviewed changes

Do proper semver test and use fastify-warning

a002268

humphd mentioned this pull request Oct 1, 2020

Add note about codes needing to be uppercase to README fastify/process-warning#4

Merged

4 tasks

mcollina reviewed Oct 1, 2020

View reviewed changes

Remove semver dependency, always warn for use of .connection

07feea1

jsumners approved these changes Oct 1, 2020

View reviewed changes

mcollina approved these changes Oct 1, 2020

View reviewed changes

mcollina merged commit a0838df into fastify:master Oct 1, 2020

humphd mentioned this pull request Oct 1, 2020

Deprecated API usage (#2575) fastify/fastify#2594

Merged

4 tasks

mcollina mentioned this pull request Oct 4, 2020

Broken usage with .inject fastify/fastify-static#146

Closed

airhorns mentioned this pull request Oct 5, 2020

Expose a mock socket that is an EventEmitter like a real one would be #101

Merged

briceruzand mentioned this pull request Oct 29, 2020

Use req.socket instead of req.connection #8 jshttp/forwarded#9

Closed

Uh oh!

fix: add missing request.socket (#87) #99

fix: add missing request.socket (#87) #99

Uh oh!

Conversation

humphd commented Sep 24, 2020

Checklist

Uh oh!

Eomm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd commented Sep 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcollina commented Oct 1, 2020

Uh oh!

humphd commented Oct 1, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

humphd commented Oct 1, 2020

Uh oh!

jsumners left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants