Skip to content

Proxy: add websocket support#596

Closed
gutenye wants to merge 1 commit intofacebook:masterfrom
gutenye:websocket
Closed

Proxy: add websocket support#596
gutenye wants to merge 1 commit intofacebook:masterfrom
gutenye:websocket

Conversation

@gutenye
Copy link
Copy Markdown

@gutenye gutenye commented Sep 7, 2016

No description provided.

@gaearon
Copy link
Copy Markdown
Contributor

gaearon commented Sep 7, 2016

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

@mathieu-pousse
Copy link
Copy Markdown

mathieu-pousse commented Sep 7, 2016

hope this works and drops the ws limitation !

@gutenye
Copy link
Copy Markdown
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 notifications@github.com
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
Copy Markdown

talkingtab commented Sep 11, 2016

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

@cloudmu
Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread scripts/start.js
ws: true,
});
devServer.use(mayProxy, proxyMiddleware);
devServer.listeningApp.on('upgrade', proxyMiddlware.upgrade);
Copy link
Copy Markdown
Contributor

@gaearon gaearon Sep 16, 2016

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
Copy Markdown
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
Copy Markdown
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

1 more reviewer

@gaearon gaearon gaearon requested changes

Reviewers whose approvals may not affect merge requirements

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