Skip to content

Proxy: add websocket support #596

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Proxy: add websocket support #596

wants to merge 1 commit into from

Conversation

gutenye
Copy link

@gutenye gutenye commented Sep 7, 2016

No description provided.

@gaearon
Copy link
Contributor

gaearon commented Sep 7, 2016

Can you please specify the test plan for this change? Please see CONTRIBUTING.md.

@mathieu-pousse
Copy link

hope this works and drops the ws limitation !

@gutenye
Copy link
Author

gutenye commented Sep 9, 2016

Yeah. I'll take a look at it when I have some time.

On Thu, Sep 8, 2016 at 6:05 AM, Mathieu POUSSE [email protected]
wrote:

hope this works and drops the ws limitation !

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#596 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAXCyEfbgipiWFOVZ6FAnqeTBsfg4dkuks5qnzU0gaJpZM4J2cxF
.

JavaScript, Go, Linux and Open Source

@ghost ghost added the CLA Signed label Sep 10, 2016
@talkingtab
Copy link

+1 for this and sooner rather than later. My site(s) use the websocket interface extensively

@cloudmu
Copy link
Contributor

cloudmu commented Sep 12, 2016 •
edited
Loading

FYI: socket.io works with the proxy already (by falling back on xhr polling) if that's a good enough alternative.

@gaearon
Copy link
Contributor

gaearon commented Sep 16, 2016

Can I ask somebody in this thread to review this change and verify it works? There is no test plan in this PR so I have no idea how to check if the fix is correct.

gaearon
gaearon reviewed Sep 16, 2016
View reviewed changes
scripts/start.js
ws: true,
});
devServer.use(mayProxy, proxyMiddleware);
devServer.listeningApp.on('upgrade', proxyMiddlware.upgrade);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a typo: proxyMiddlware but variable is proxyMiddleware.
Have you verified this code works by running it?

gaearon
gaearon suggested changes Sep 16, 2016
View reviewed changes
Copy link
Contributor

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Needs variable name fix and a test plan.

@gaearon
Copy link
Contributor

gaearon commented Sep 30, 2016

I’m closing because this doesn’t work and it seems like it wasn’t tested.
Please address the review comments and I’ll be happy to review it again!

@gaearon gaearon closed this Sep 30, 2016
@agido-freudenreich agido-freudenreich mentioned this pull request Nov 11, 2016
Closed
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gaearon gaearon gaearon requested changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gutenye @gaearon @mathieu-pousse @talkingtab @cloudmu @fson @facebook-github-bot