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

Dont append port 80 to the socketUrl #3

Merged
merged 2 commits into from
Dec 11, 2018
Merged

Conversation

Munksgaard
Copy link
Contributor

Intended to fix #2

@wking-io
Copy link
Owner

wking-io commented Dec 4, 2018

Thanks for the PR to fix the problem that you found! Couple of questions before I merge:

What tests did you run to make sure it still works in all expected scenarios? ( Sorry the test suite in this lib is not the best yet! )

Did you test that live reload still works in both http and https?

@Munksgaard
Copy link
Contributor Author

Thanks for the PR to fix the problem that you found! Couple of questions before I merge:

What tests did you run to make sure it still works in all expected scenarios? ( Sorry the test suite in this lib is not the best yet! )

Did you test that live reload still works in both http and https?

I tested that the live reload worked on both http and https when running through the ngrok tunnel, and it also seems to work when I am accessing my server locally (through http).

The result of my changes is mostly a simplification of what was there before. I don't think it is necessary for the socketUrl to actually have port information in it, which seem to have been the assumption before, but I am not in any way an expert, so it is very possible that there are edge cases I haven't thought about.

@wking-io
Copy link
Owner

wking-io commented Dec 6, 2018

I am also not in anyway an expert haha! I will try and do some research, but if I do not find anything I will merge! :)

@Munksgaard
Copy link
Contributor Author

I am also not in anyway an expert haha! I will try and do some research, but if I do not find anything I will merge! :)

Sounds good. Thanks for your work, elm-serve (and elm-live) are great tools to work with :-)

@Munksgaard
Copy link
Contributor Author

Any progress on this?

@wking-io wking-io merged commit 9bcb905 into wking-io:master Dec 11, 2018
@wking-io
Copy link
Owner

Yeah researched and asked around and as far as I could tell it would not be a problem. Seems websocket ports fill automatically. So I guess we will find out! 👍

@Munksgaard Munksgaard deleted the patch-1 branch December 11, 2018 15:00
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.

Incorrect handling of https addresses
2 participants