Skip to content

Conversation

BlazingAsher
Copy link

@BlazingAsher BlazingAsher commented Apr 27, 2021

Fixes #152

Checklist

@mcollina
Copy link
Member

Can you please add a unit test?

@BlazingAsher
Copy link
Author

To be honest, I'm not sure what to even test for. It seems to work with this in certain projects but requires the change in others.

@mcollina
Copy link
Member

What was the bug? Have you got a reproduction that you used to check if this fixed your problem? Port that to be a unit test.

@BlazingAsher
Copy link
Author

The bug was that the prefix was not getting replaced before the request is forwarded to upstream.

I have been using this library without issue in another project, but a new one that I just created following the example in the README wasn't working.

I'll take a closer look to see if there are any differences, but the implementation in the units tests look to be the same as the one I am using.

@mcollina
Copy link
Member

There is likely some difference ;).

@BlazingAsher
Copy link
Author

Okay, it seems that my TypeScript watcher stopped working so I was running an older version during my testing. The issue is caused by the fastify-websocket library. It hijacks the handlers and binds the internal fastify server object instead of the public API.

Let me know how you'd like me to proceed with this, since I don't think this is normal behaviour and you may not want to deal with it, but I am happy to write a test if you'd like to incorporate this change.

@mcollina
Copy link
Member

Does the fix resolve your problem then?

@BlazingAsher
Copy link
Author

Yes, the proposed changes fixes it.

@mcollina
Copy link
Member

Then add an automated test that failed before the change.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Now that I see the test, I'm starting to think I might be wrong and the bug might actually be in fastify-websocket.

What do you think?

@BlazingAsher
Copy link
Author

Yeah, that's what I'm thinking, so I'm not too sure what you want to do with it. The change does fix the problem and it seems there is already use of fastify.prefix instead of this.prefix anyways, but I'm not sure whether you want to test for something that isn't really supposed to happen in the first place.

@mcollina
Copy link
Member

Take a look at fastify/fastify-websocket#122, I think it's the correct fix.

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.

fastify-http-proxy not removing prefix

2 participants