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

Use document.currentScript for deriving socket connection url #1994

Closed
wants to merge 3 commits into from

Conversation

dburrows
Copy link

When loading a create-react-app bundle from another domain everything works ok except the auto-page reload. The socket connection fails because webpackHotDevClient uses window.location to derive the socket connection url. This commit uses document.currentScript (if available) to derive the SockJS connection url details.

There's a load of ways to find the current executing script in browsers that don't support document.currentScript but they all seemed a bit fragile, so I've made a pragmatic decision to just use currentScript - main downside is that this won't work in IE but that's ok for a dev tool I think. Full support details outlined here: http://caniuse.com/#feat=document-currentscript

I've tested this by loading a normally created app and then loading the same app bundle from another domain, both worked.

P.S. Feel free to close this if you don't think it's a good idea, I'd implemented it anyway and it's quicker to open a PR with it than explain what I wanted to do ahead of time.

…tion url

When loading a create-react-app bundle from another domain everything works ok except the auto-page reload. The socket connection fails because webpackHotDevClient uses window.location to derive the socket connection url. This commit uses document.currentScript (if available) to derive the SockJS connection url details.
@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.

@Timer
Copy link
Contributor

Timer commented Apr 18, 2017

Hey, thanks for the PR!
Does #1301 or #1588 solve this? Just curious.

@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!

@dburrows
Copy link
Author

I think #1588 is a good idea, but this PR could be implemented on top of that and still be helpful, HOST/PORT would always override it.

Looking at the original issue #853, I think my fix would fix that issue as long as you didn't expect it to work in IE!

@Timer
Copy link
Contributor

Timer commented Apr 19, 2017

I'm not sure if I understand how this fixes the underlying issue, afaik the browser doesn't know a connection is being proxied.

@dburrows
Copy link
Author

The browser doesn't need to know, it just needs to make a socket connection to the same domain that the bundle is loaded from, probably easier with an example:

Say create-react-app is running on localhost:3000 and generating a bundle at localhost:3000/static/js/bundle.js.

We then have a dev server running on localhost:3001 that serves our html (with bootstrap data etc.) and api, and the initial page loads the bundle from localhost:3000/static/js/bundle.js.

Current behaviour: webpackHotDevClient uses the document.location e.g. localhost:3001 to try to connect to the hot dev websocket server when the websocket server is actually running on localhost:3000.

New behaviour: webpackHotDevClient uses document.currentScript.src to work out the websocket url and, as webpackHotDevClient is part of the bundle loaded from 'localhost:3000', it correctly connects to the socket on localhost:3000

@sktt
Copy link

sktt commented Apr 20, 2017

@dburrows I don't think this works fully unless your publicPath matches the path of the of hotUpdateChunk. If it does not match, the fallback is to hardreload the page. Try to for example edit one of your stylesheets and verify that the update is applied without a reload.

have look at https://github.com/webpack/webpack/blob/54aa3cd0d6167943713491fd5e1110b777336be6/lib/JsonpMainTemplate.runtime.js#L27

This was what was blocking me from opening a pr like this:
Serving the app from an origin that is different from the devServer's will also likely break publicPath which is normally /. This would also need to be set to origin of document.currentScript.src ie the devServer.. While publicPath can be set in runtime, it's usually expected to be configured through the config file.

Edit: It could also be possible to set publicPath programatically from scripts/start.js, this is where we determine what port to use for the devServer. but it would mean modifying the config files of the devServer and the webpack config from start.js which feels a little bit outside of its scope......

@dburrows
Copy link
Author

@sktt ok, I guess a reload is better than nothing though?

@sktt
Copy link

sktt commented Apr 21, 2017

It is better with nothing than a half implemented feature

edit: sorry about that. I mean, it would be really nice to have it fully working.. This wouldnt though break anything so ... you might actually be right

@gaearon
Copy link
Contributor

gaearon commented Jan 9, 2018

I don't think we'll merge a half-working solution because it will be harder for people to figure out why it doesn't work. Whereas in the current case it's pretty clear we just don't support this use case.

Thanks for PR though!

@gaearon gaearon closed this Jan 9, 2018
@lock lock bot locked and limited conversation to collaborators Jan 20, 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.

5 participants