Support webpack-hot-middleware

[blainekasten]

This is a superset to #12 which was closed because I force-pushed on my branch. Apparently that is a no-no.

High Level

So, I'm not 100% certain this is the way to go. But it does seem to work. The goal of this PR is to support webpack-hot-middleware.

Involved:

• Setup an example site to test with
• Hook up the webpack-hot-middleware client to use the custom reporter in the code
• Do not create a socket if we are not in the webpack-dev-server world

Why this change

webpack-dev-server injects a runtime that has exposes a global of __webpack_dev_server_client__. When running an express server with webpack-dev-middleware that global is not available, but a different one will become available. I'm not sure why it's not immediately available, but in all my tests it's exposed later (hence the setTimeout + retries).

The __whmEventSourceWrapper global is an object with the whm path as the key. It's always a single key object and it's dynamic, so just grabbing the first value gives us access to the EventSource, in which we add our listener. It seems to follow the same data spec because the messageHandler continues to work!

I also commented out the sock-client code because it does not seem properly implemented. The API for when sock-client is the resolved SocketClient causes uncaught errors.

Looking for feedback! I'm by no means a webpack expert, so I could surely be off here. Just wanted to get this moving because fast refresh is going to be great! Thanks for the library 😄

[swashata]

Hi,

I am trying to use this with a custom webpack-dev-middleware and webpack-hot-middleware server. This seems to work. But for some reason, I am getting two errors:

1. At build time, I get

React Refresh Plugin: The plugin is unable to detect transformed code from react-refresh. Did you forget to include "react-refresh/babel" in your list of Babel plugins? Note: you can disable this check by setting "disableRefreshCheck: true".

Although I have the babel plugin, otherwise it shouldn't have worked.

2. ReferenceError: messageHandler is not defined. I think it has something to do with what to do when there is an error (overlay?). Is this something we need to take care of when implementing?

[swashata]

Also there seems to be a request on /sockjs-node/info?t=1580741729089 from sockjs client. Is it also coming from this module?

[blainekasten]

@swashata it wasn't ready. It was very much a WIP and I hadn't touched it for a while. It's much more stable now

[blainekasten]

@pmmmwh good news, this now works with hot-middleware! A couple questions though in the inline comments to sort out

[blainekasten]

So the main issue that webpack-hot-middleware experiences, is that since __webpack_dev_server_client__ does not get injected, we trigger this else statement. Which also ends up crashing by failing to connect. After the third time the Socket tries to reconnect, it returns html, which then it's internals try to call res.json() and crash, prompting the Error screen.

So i'm not really sure what the solution is here. Nor do I really understand what the use case for this custom SocketClient is. Do people really use this? I wonder if they do, if we could require a configuration option to be passed in for this.

Would love your thoughts here, as this is the main blocker for adoption in webpack-hot-middleware

[pmmmwh]

The custom SockJS client is for backwards compatibility with WDS versions before 3.8.0. Before that, WDS will return a raw SockJS client (onClose is expressed as onclose, and onMessage are expressed as onmessage, thus breaking code below).

I guess we could fix the compatibility here by detecting the methods listed above and add the wrapper only when needed as a workaround.

[blainekasten]

@pmmmwh good to know. I didn't know that. So let me explain why this wasn't working with webpack-hot-middleware with your current implementation.

In this if/else statement the else statement is always hit when using webpack-hot-middleware because it does not inject that global variable __webpack_dev_server_client__.

When we hit that else statement, the sockjs socket is initialized and starts trying to create a connection to some endpoint. This endpoint is never valid in a webpack-hot-middleware setup. So what happens then is that it tries to connect to that endpoint a couple times (has a retry mechanism internally). Then when it finally decides it can't connect, it emits an error which triggers the ErrorOverlay. 🚫

So currently any use of this plugin in webpack-hot-middleware always results in the ErrorOverlay being displayed for a SockJS connection error.

Thus the fix is to only initialized the __webpack_dev_server_client__ if we are in a webpack-dev-server mode. This is easy when the global variable is around because we can immediately detect it and know that we should use it to listen for compile messages.

When we are in webpack-hot-middleware, we do not have this sort of mechanism. The global variable it does install is not installed immediately and makes it hard to react to. The best way to do this is just to import the package directly and use it. This would be a noop in other environments because the client wouldn't do anything. But still we guard against it.

Lastly, this leave the legacy webpack setup. I was hoping there was some heuristics at build time to know if that setup was involved, but I couldn't find anything. The best option I see is that we had a configration flag for using legacy websockets. This is the implementation I have currently taken and would like your feedback and discussion on.

The new configuration is new ReactRefreshPlugin({ useLegacyWebsockets: true }). What do you think about this approach?

[pmmmwh]

I think a reasonably heuristic to test for the legacy socket is via client.onmessage - in WDS < 3.8 it will be defined, and in WDS >= 3.8 it will be undefined (client.onMessage will be defined, instead).

There are actually two problems here (sorry for the confusion 😭):

1. Support for legacy socket implementation in WDS (which can be detected via the heuristic above)
2. Support for non-inline mode WDS usage - as seen in this line, WDS do not inject the client (because the whole runtime is injected via an iframe). While this is non-suggested usage, I think your implementation actually solved this issue.

Nonetheless, I think both are out of scope for this PR, but since you've already added the flag, maybe rename that to useLegacyWDSSockets to indicate that it is exclusive to WDS? Then we can get this PR merged, and I can take on the first issue in another PR 🎉

[blainekasten]

Awesome! I'll add this right now and add it to the documentation! Give me ~30m!

[blainekasten]

Done! Ready for you to review/merge. Thanks @pmmmwh !

[pmmmwh]

The custom SockJS client is for backwards compatibility with WDS versions before 3.8.0. Before that, WDS will return a raw SockJS client (onClose is expressed as onclose, and onMessage are expressed as onmessage, thus breaking code below).

I guess we could fix the compatibility here by detecting the methods listed above and add the wrapper only when needed as a workaround.

[blainekasten]

Awesome!! Thanks for the merge, looking forward to the release @pmmmwh ❤️

[pmmmwh]

Released in v0.2.0 🎉