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

Respect http[s] protocol when making socket.io middleware #976

Merged

Conversation

damian0815
Copy link
Contributor

Closes #973

Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

The splitting of hostname and port is a remnant of old code. Let's just do this:

  const { origin } = new URL(window.location.href);

  const socketio = io(origin, {
    timeout: 60000,
  });

Can you please update the PR accordingly? I cannot make code suggestions.

@damian0815
Copy link
Contributor Author

try that

@damian0815 damian0815 changed the title Respect http[s]x protocol when making socket.io middleware Respect http[s] protocol when making socket.io middleware Oct 8, 2022
Copy link
Collaborator

@psychedelicious psychedelicious left a comment

Choose a reason for hiding this comment

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

Just tested the revision, all good. Thanks!

@ookami125
Copy link

This change also worked for me getting it to run in docker behind traefik.

@colemickens
Copy link

Would it be appropriate to go ahead and regenerate the frontend and commit it, since that's a thing that's going on in this repo?

Very surprised this isn't merged or affecting more folks.

@gfosco
Copy link

gfosco commented Oct 18, 2022

Ran into this and resolved it a bit differently, replacing http:// with just //.. but this origin change is better.

👍

@hipsterusername
Copy link
Member

@damian0815 - Can you rebase and also do a yarn build? I'll merge once everything is ready

@damian0815
Copy link
Contributor Author

on it

@hipsterusername
Copy link
Member

@damian0815 - It's possible that @psychedelicious already took care of it. Mentioned that his bundle includes these changes. Given that, if you rebase, I'll go ahead and merge

@damian0815
Copy link
Contributor Author

@hipsterusername rebased and yarn built

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.

None yet

6 participants