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

Adds HOST and PORT env variables to react app so websocket #1588

Closed
wants to merge 2 commits into from

Conversation

scisci
Copy link

@scisci scisci commented Feb 18, 2017

Adds HOST and PORT env variables to react app so websocket can use them instead of window.location.

This addresses the issue listed here: #853

This can be tested by adding a .env file to an app with the following content:

HOST=127.0.0.1
PORT=3000

Then run the app by visiting http://localhost:3000. If you look at the network requests, you will see the websocket is using 127.0.0.1 and not localhost.

@facebook-github-bot
Copy link

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@scisci
Copy link
Author

scisci commented Feb 18, 2017

Added CLA

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

So does this fix the problem with pow specifically?

@scisci
Copy link
Author

scisci commented Feb 24, 2017

I don't use pow so I can't answer for certain. Maybe @thom-nic could chime in.

But I think it should because create react app would be listening on localhost:3000 for the websocket and that is what would be requested from the react app as long as its specified under host/port env vars.

Currently if its getting served through foo.dev via window.location, the web socket would get denied by the pow server.

@gaearon gaearon added this to the 0.10.0 milestone Feb 24, 2017
@gaearon
Copy link
Contributor

gaearon commented Feb 24, 2017

OK, tagging it for consideration for 0.10.
I don't understand the issue very well yet so it would help if more people could chime in.

@scisci
Copy link
Author

scisci commented Feb 25, 2017

The issue is if you are running a separate web server as your main backend for a project and are proxying requests to the react app bundle.js through it.

As I said in the issue, I was able to overcome this issue by implementing a websocket proxy on my server, so its not crucial for me anymore, but it seems to make sense. To assume that window.location is the server hosting the websocket seems optimistic. But I see your point about create react app only for simple setups.

@cr101
Copy link
Contributor

cr101 commented Feb 25, 2017

When building a WordPress theme using React and the WordPress API you need to run a separate web server (Apache or Nginx) as the main backend.

@fabiosussetto
Copy link

fabiosussetto commented Mar 1, 2017

This is going to be helpful for me as well as I'm trying to use react-create-app with a Django backend where I need to render the index.html file in the public dir server side (mainly to pass some initial data and settings to React). At the moment I have the problem where the websocket is trying to connect to http://localhost:8000 (Django) instead of http://localhost:3000 (Webpack dev server).
I confirm this fix would solve my issue.

@thom-nic
Copy link

thom-nic commented Mar 1, 2017

Yes that sounds like it would solve my initial request. I took to heart that create-react-app is for trivial setups and quickly ejected and customized things, so I can't say I'm facing the original situation anymore.

@sktt
Copy link

sktt commented Apr 27, 2017

Can we also, for the webpack dev config, have publicPath changed from / to http://${process.env.HOST}:${process.env.PORT}/. This would enable fetching the hot module chunks without hard reloading when index document server origin is different from the dev server

@dan-kez
Copy link

dan-kez commented May 7, 2017

Thanks for making this PR! I ran into an issue with developing chrome extensions where window.location.protocol is set to chrome-extension: causing an error with SockJS.

This can be fixed by adding the following:

var connection = new SockJS(url.format({
-  protocol: window.location.protocol,
+  protocol: process.env.REACT_APP_SOCKJS_PROTOCOL || window.location.protocol,
...
}));

Edit:
Also, loading of static content will also become an issue in dev since publicPath in webpack.config.dev.js is hardcoded to /. There may be a different workaround that I'm unaware of.

@sktt
Copy link

sktt commented May 7, 2017

How about changing this line to procotol: process.env.HTTPS ? 'https' : 'http'? That would fix your problem too, @dmk255, right?

edit: This should probably also be done for the publicPath change that I suggested above

@niekert
Copy link

niekert commented Jan 6, 2018

I would also really like to see this feature added. My situation is the same as others described, I'm loading the React bundle from a PHP app that runs on a .internal domain by injecting a script to http://localhost:3000/bunlde.js. Because the PHP app is being migrated to React, the React bundle is built in a separate CRA project so we can completely get rid of the PHP backend eventually.

For people with the same use case who'd like to get this working, I published an npm package which contains a drop-in replacement for webpackHotDevClient with support for this. https://www.npmjs.com/package/react-dev-utils-custom-hmr

Simply install the package and change require.resolve('react-dev-utils/webpackHotDevClient') to require.resolve('react-dev-utils-custom-hmr/webpackHotDevClient') in webpack.config.dev and add

REACT_APP_HMR_HOSTNAME=localhost
REACT_APP_HMR_PORT=3000

to your .env file.

If HMR still doesn't work but the page refreshes instead. Make sure you have set the CORS headers in webpackDevServer.config.js by adding

headers: {
  'Access-Control-Allow-Origin': '*', // * is not recommended
}

@thomasbertet
Copy link

thomasbertet commented Jan 18, 2018

To add some to this discussion, we might consider changing this fetch since it will call the proxy instead of the app, thus you won't be able to click to open the editor. We will also need to make this request in "no-cors" mode to make it work (I believe).

Since this PR seems blocked by some architectural design, what can we do to help this moving forward ?

@dburrows
Copy link

Agree with @thomasbertet, I'm more than willing to devote a fair bit of time to get this implemented, we just need to know what direction @gaearon & @Timer want the implementation to go in.

@thomasbertet
Copy link

Also, if this will not be considered in CRA 1.x (maybe you did some in the next branch), that's fine by me, but please let us know :) We might then find an alternative (in my case another fork I'm afraid 🙄 )

@djalmaaraujo
Copy link

djalmaaraujo commented Jan 26, 2018

@niekert For your fast solution, you are assuming that the app is ejected, right?

btw, it works.

Also, what is the repo for this custom hot reloader? yourusername/react-dev-utils-custom-hmr is not a repo...

@djalmaaraujo
Copy link

djalmaaraujo commented Jan 27, 2018

For whom may want fix this using @niekert solution, but without ejecting, use this:

  1. https://gist.github.com/djalmaaraujo/0d9b2243518b68f239fc916111b2bcac

  2. Add these two devDependencies:

"react-dev-utils-custom-hmr": "^3.1.0",
"replace-string": "^1.1.0"
  1. And in your package.json, add this:
"start": "node fix-hot-reload.js && sleep 4 && react-scripts start",

Since it's async, I added the sleep.

Hacky, but still better than ejecting.

Arahir pushed a commit to habx/create-react-app that referenced this pull request Jun 4, 2018
Arahir added a commit to habx/create-react-app that referenced this pull request Jun 4, 2018
@maisui99
Copy link

it seems that this feature is still not added...

@Timer Timer removed this from the 2.x milestone Nov 2, 2018
@robertvansteen
Copy link
Contributor

@Timer no plans to add this in 2.x anymore?

@younes0
Copy link

younes0 commented Nov 21, 2018

We didn't switch yet to Create React App here because of this issue.
We use (unfortunately) different backends to serve the legacy php frontend and react app, so we need to specify ws host/port to benefit from hot reloading

@yordis
Copy link

yordis commented Jan 28, 2019

Hey @Timer by any change will this make it into CRA? We have some Rails that still serving some React code but we use CRA dev server so we need this integration.

Also, I would agree on using other keys outside HOST and PORT scoping.

NikkoCruz1989 pushed a commit to NikkoCruz1989/create-react-app that referenced this pull request Jul 15, 2019
NikkoCruz1989 pushed a commit to NikkoCruz1989/create-react-app that referenced this pull request Jul 22, 2019
@atifsyedali
Copy link

After 2 years, I still don't understand what the issue with the original PR was. Why was there architectural changes needed? The PR only uses PORT and HOST env. variables if they are there and if not then falls back to window.location.

Since then, I have seen many universal/ssr projects either fork react-dev-utils for this, or the replace-string build time hack provided above. I don't like either approach as I would like to not think about keeping an up-to-date react-dev-utils or monkey patching it.

@Timer @gaearon If you guys can please provide some guidance here it would be greatly appreciated.

@robertvansteen
Copy link
Contributor

Superseded by #7750

@lock lock bot locked and limited conversation to collaborators Oct 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.