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

Missing socket #87

Closed
Eomm opened this issue Apr 21, 2020 · 3 comments · Fixed by #99
Closed

Missing socket #87

Eomm opened this issue Apr 21, 2020 · 3 comments · Fixed by #99

Comments

@Eomm
Copy link
Member

Eomm commented Apr 21, 2020

Hi,
I'm writing some test for a Fastify plugin that uses some koa handler to manage routes.

The test is failing due this check of koa lib:

if (this.socket.encrypted) return 'https';

https://github.com/koajs/koa/blob/master/lib/request.js#L402

Considered that:

  • the final solution would be to implement a native plugin for Fastify
  • light my request says "Does not use a socket connection.."
  • I can use listen instead of inject to write my tests

Do you think adding a this.socket = {} to the light-my-request is a bad idea?
How would you cover this use case?

@humphd
Copy link
Contributor

humphd commented Sep 23, 2020

I also need this to get tests passing for the change in fastify/fastify#2575, switching from .connection to .socket.

I tried doing a naive switch in

this.connection = {
remoteAddress: options.remoteAddress || '127.0.0.1'
}
from this.connection = {...} to this.socket = {...}, but it's clearly going to need more than that.

I also found that doing so broke other modules that depend on the request still having a .connection property (e.g., forwarded).

@mcollina
Copy link
Member

You need add: this.socket = this.connection right after that block.

@humphd
Copy link
Contributor

humphd commented Sep 23, 2020

Thanks @mcollina, I'll do that and send a PR.

humphd added a commit to humphd/light-my-request that referenced this issue Sep 24, 2020
mcollina pushed a commit that referenced this issue Oct 1, 2020
* fix: add missing request.socket (#87)

* Add test, update typing, add connection property

* Do proper semver test and use fastify-warning

* Remove semver dependency, always warn for use of .connection
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 a pull request may close this issue.

3 participants