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

If sockHost is not specified, overlay will try to connect to 3rd party server hosting other scripts on the page #554

Closed
erikt9 opened this issue Dec 31, 2021 · 9 comments · Fixed by #630

Comments

@erikt9
Copy link

erikt9 commented Dec 31, 2021

Using the latest version of the plugin (0.5.4)

I'm initializing like:

new ReactRefreshWebpackPlugin({
          overlay: {
            sockPort: 1887
          }
});

The url of the page:
http://localhost:3000/

Webpack dev server is on localhost:1887.

The console will show an error opening a web socket to a third party server (e.g., wss://googleads.g.doubleclick.net:1887/ws, but the host of other third party scripts used on the page is seen frequently as well) instead of localhost. According to the docs, the default should be window.location.hostname. This error seems to be related to usage of this plugin (changing settings as described below resolves the issue).

While not happening every time I load a page, the error happens very frequently. Manually setting sockHost to 'localhost' makes it so the connection always goes to localhost, however it uses wss instead of ws (which I think is related to the third party scripts being loaded with https instead of http like the main page). So I also need to hardcode sockProtocol to 'ws'. Once I do this it works 100% (but I shouldn't need to according to the docs, and I don't want to hardcode these values).

The web socket established by webpack-dev-server (4.7.2) always establishes correctly -- only the socket established by this plugin has intermittent issues.

@erikt9
Copy link
Author

erikt9 commented Dec 31, 2021

Reading through the code, it seems like setting sockHost to '0.0.0.0' triggers using window.location.hostname, otherwise it will try to use document.currentScript. Given that document.currentScript doesn't work well when async scripts are used on the page (a common use case), this either needs to be fixed to follow the current docs that state window.location.hostname is used by default, or the docs need to be updated to describe this unexpected behavior.

@pmmmwh
Copy link
Owner

pmmmwh commented Jan 6, 2022

It should probably have the same behaviour as WDS, I'll take a look at why this was breaking.

@pmmmwh
Copy link
Owner

pmmmwh commented Apr 4, 2022

Hi, it's been a while but I tried looking at this and I cannot see how our implementation would break - in particular, if document.currentScript is returning a 3rd-party script, WDS's implementation should have also broke. Can you provide a reproducible example for debugging? Are you using pure ESM by any chance?

@erikt9
Copy link
Author

erikt9 commented Apr 18, 2022

I think webpack works for me and this plugin has the issue due to how I load files on the page (webpack stuff gets loaded after document.onload and doesn't have any third party scripts coming in at the time). At the very least, the documentation for sockhost should be updated as it states that the default host is window.location.hostname when it's actually document.currentScript and to achieve the window.location.hostname behavior you need to put in the nonobvious 0.0.0.0 value.

@pmmmwh
Copy link
Owner

pmmmwh commented Apr 19, 2022

Can you provide a reproducible example for me to better understand the situation? The options try to mimic the behaviour of WDS as close as we can get, and under normal circumstances host from document.currentScript is likely a better default than location.hostname. We are also moving away from our own custom implementation of WDS and instead use an exported instance from them, so in the near future if it works there it would also work here (this requires WDS 4.8+)

@erikt9
Copy link
Author

erikt9 commented Apr 19, 2022

@pmmmwh I'll see what I can do for an example. In my specific case I have an externalized react (https://github.com/pmmmwh/react-refresh-webpack-plugin/blob/main/docs/TROUBLESHOOTING.md#externalising-react). I bundle the plugin with the react library itself. Our page loading order goes something like:

  1. Necessary synchronous scripts
  2. Less important async scripts (including both react and this plugin as well as other 3rd party scripts)
  3. The webpack bundle scripts which depend on react from step 2 being present as an external.

The plugin is loading in step 2 along with google-analytics, facebook, etc. and thus document.currentScript often picks up one of them.

I know this is more of an edge case for you and is somewhat atypical. I'm not arguing that window.location.hostname should or should not be the default versus document.currentScript. I just want the docs to be accurate, which means either the docs need to change to match the code or vice versa.

We both agree the documentation for sockhost is incorrect, right? We both agree that getting the hostname from document.currentScript != window.location.hostname, right? We both agree it would be helpful to document that the address '0.0.0.0' is a special case triggering unique behavior?

I just feel incorrect docs are worse than no documentation at all. It took me a couple hours to figure out what was happening because I believed the documentation and I don't want that to happen to other people. I'll be happy to make a pr to update the documentation if you like.

@pmmmwh
Copy link
Owner

pmmmwh commented Apr 19, 2022

The point is, document.currentScript IS SUPPOSED TO WORK even with async scripts. Yes, document.currentScript's src might not always equal to window.location.hostname, but the issue seems bigger than documentation because even if the documentation is wrong, it should have picked up some sensible host (if script is on the same host as page, or not) and not something random from other third party scripts. Are you using a browser where document.currentScript doesn't work reliably when async (Firefox being 1 of them)? We might have to fix async detection and not fallback to using all scripts on the page and find the last one.

I'm not opposing the idea of changing the documentation, in fact I would welcome PRs for it, but I would also like to highlight that these options are soon to be deprecated because direct integration with WDS is now possible.

(However, I also think this plug-in's chunks should load AFTER WDS anyways, and potentially not async, because implicitly there's a dependency on socket implementations to be completely connected before we try anything.)

@pmmmwh
Copy link
Owner

pmmmwh commented Apr 19, 2022

We both agree the documentation for sockhost is incorrect, right? We both agree that getting the hostname from document.currentScript != window.location.hostname, right? We both agree it would be helpful to document that the address '0.0.0.0' is a special case triggering unique behavior?

Not to say the current documentation text is correct but to give more context:

  • In most cases, if you host your scripts on the same local machine without using a proxy, document.currentScript's src do equal window.location.hostname. The docs go over this point by saying that if you use something else, such as a proxy, you should customise this option.
  • The docs imply that we should try to get to a sensible host ALWAYS, but in this case it did not, which is a BUG, and exactly why I asked for a reproduction, because it will help me understand the situation more and potentially add to TROUBLESHOOTING.md.
  • 0.0.0.0 is a special case from WDS, and I think I failed to convey properly in the docs that these options (host/port/path) should exhibit the same behaviour as WDS unless otherwise specified - in that sense all special cases should be referred back to WDS' docs, and we should point that out.

@pmmmwh
Copy link
Owner

pmmmwh commented Apr 25, 2022

This will be fixed in #630 and should be released soon.

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 a pull request may close this issue.

2 participants