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

Support for webpack-hot-middleware #12

Closed
wants to merge 0 commits into from

Conversation

blainekasten
Copy link
Contributor

@blainekasten blainekasten commented Dec 7, 2019

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.

The meat of the changes here are in this file: src/runtime/createSocket.js

The other changes is just setting up examples to have a webpack-dev-server and webpack-dev-middleware setup.

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 😄

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 8, 2019

Thanks for the PR!

Some high level comments:

I'm not sure why it's not immediately available, but in all my tests it's exposed later (hence the setTimeout + retries).

This is because WHM sets the socket object on window when the client script first runs. However, since this plugin injects entries (because we need to run before any imports to React), the socket code will be guaranteed to execute before the WHM client can execute. I think using setTimeout to ping for the client is fine in that regard (another way to do that is to move the overlay script to the last entry, but I will have to check if it causes other timing issues).

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'm not sure about that. Looking at the code of WHM, it seems that the socket emits in a different way. The events seems to be built, building and sync, and errors/warnings are inside the nested tree. We might have to massage the data before it is consumable by the error overlay. (Unfortunately I don't have time to test out this branch for now, so I'm happy to be proven wrong.)

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.

That's my bad. The API changed between WDS versions and I forgot to update it. Will fix this in a separate PR! Thanks for raising this up.

Another note:
I am updating the examples directory (to include CRA examples, primarily), so this branch might face some merge conflicts soon. I can walk you through it when it happens.

@pmmmwh pmmmwh added the enhancement New feature or request label Dec 8, 2019
@blainekasten
Copy link
Contributor Author

@pmmmwh thanks for the detailed feedback!

So it sounds like the main action item here is to figure out this comment.

I'm not sure about that. Looking at the code of WHM, it seems that the socket emits in a different way. The events seems to be built, building and sync, and errors/warnings are inside the nested tree. We might have to massage the data before it is consumable by the error overlay. (Unfortunately I don't have time to test out this branch for now, so I'm happy to be proven wrong.)

Is there anything else to figure out? I can dig into this and make sure the error overlay gets properly rendered.

I am updating the examples directory (to include CRA examples, primarily), so this branch might face some merge conflicts soon. I can walk you through it when it happens.

Do you have an ETA on that? If this is ready first do we want to take this and sort out the merge conflicts on your side? or could I delete my examples changes after this is ready so we can merge this? We want to implement this in gatsby and this is the main blocker.

@pmmmwh
Copy link
Owner

pmmmwh commented Dec 10, 2019

Is there anything else to figure out? I can dig into this and make sure the error overlay gets properly rendered.

That's the only thing that I think will be broken right now, apart from the timing issues.

Do you have an ETA on that? If this is ready first do we want to take this and sort out the merge conflicts on your side? or could I delete my examples changes after this is ready so we can merge this? We want to implement this in gatsby and this is the main blocker.

Just merged #16 , so it should be good to go for you :)
I think the only change is that you can remove the moved WDS related files and move WHM related files into the examples directory.

Happy to hear that the plugin is getting some use too 🤣

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants