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

Fix Socket Port option #1683

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix Socket Port option #1683

wants to merge 1 commit into from

Conversation

glingy
Copy link

@glingy glingy commented Apr 30, 2019

The socket port option documented in https://www.browsersync.io/docs/options/#option-socket did not affect anything. This allows the socket.port option to override the default port option so browser-sync can work in situations where the external port does not coincide with the internal port (ex: inside a docker container with fixed external port). With quick testing, this seems to allow the socket.port option to change the port the socket connects to from the client.

The socket port option documented in https://www.browsersync.io/docs/options/#option-socket did not affect anything. This allows the socket.port option to override the default port option so browser-sync can work in situations where the external port does not coincide with the internal port (ex: inside a docker container).
@glingy
Copy link
Author

glingy commented Apr 30, 2019

Note: helps issue #505 or at least improves the situation. I realize that in a docker container, this does not help much, but it is very helpful in other situations where browser-sync is behind a proxy.

@glingy glingy changed the title Fixed Socket Port option Fix Socket Port option Apr 30, 2019
@davidwindell
Copy link

davidwindell commented Jun 1, 2020

This would be really great and would fix the problem I described in #419, @shakyShane could you, @crlang44 or another maintainer review this? Many thanks!

@crlang44
Copy link

crlang44 commented Jun 1, 2020

I would certainly be surprised if I was a maintainer on this repo. Where do you see that?

@davidwindell
Copy link

Ah, it was on #1736, I must have mistook your acceptance of the review changes as being a maintainer

@davidwindell
Copy link

We couldn't wait for this so we've added a post-install script with sed to apply the patch:

"postinstall": "sed -i 's/var port = options/var port = socketOpts.port || options/g' node_modules/browser-sync/dist/connect-utils.js"

Works great!

@davidwindell
Copy link

@shakyShane any chance of getting this one merged?

@shakyShane
Copy link
Contributor

I'd like to get this fixed, looking for a simple reproduction :)

@glingy
Copy link
Author

glingy commented Dec 27, 2023

It's been too long, I don't have a reproduction anymore and barely remember this issue.

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