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

Add target host to new request #181

Closed
wants to merge 1 commit into from

Conversation

simensen
Copy link

Originally intended to address #170 but is an alternative fix to the solution proposed in #162. It turns out that the data in the message contains the original host header and is passed on to the target as a header. I think that rewriting it is better than removing it (proposed in #170) as it makes it possible or smee client to work behind a reverse proxy (nginx, like Laravel Valet) or some Docker environments (using something like traefik as I am).


Relatively new to typescript, Node.js, and jest at this level, so unclear if there was a better way to handle the testing. The issue I ran into was the logging output in the success handler.

Cannot log after tests are done. Did you forget to wait for something async in your test?

I couldn't find out a way to get around this issue without either 1) making logging optional, 2) including an additional optional callback so that we could call it when the response was completed 3) ???. So, I opted to fake it for now.

If there is a better way for me to do this let me know!

@simensen
Copy link
Author

simensen commented Jan 6, 2022

Happy new year! Is there anything I can do to help with this PR?

@schallinterferenz
Copy link

Hello,

I am using smee-client 1.2.3 . I inserted the "data.host = target.hostname" line locally into my index.js script and it worked like a charm!

When do you think this will be featured in the next releases?

Greets

Eduard

@gerrnot
Copy link

gerrnot commented Jul 8, 2022

@simensen : Thanks a lot, fixed it also for me.

@JasonEtco @tcbyrd I would appreciate if you would merge and release this! (As I need to currently build from this fork as a workaround)

@simensen
Copy link
Author

simensen commented Dec 6, 2022

@JasonEtco @tcbyrd Is there anything more I can do to help get this PR merged?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Feb 3, 2024

@simensen

I hope everything is working to your liking ;).

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.

4 participants