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

Fix for '[HMR] You need to restart the application!' on server sided error #730

Merged
merged 3 commits into from
Sep 27, 2018
Merged

Conversation

Bram-Zijp
Copy link
Contributor

When you make an error (e.g. undefined variable) somewhere in the server.js, you need to restart the application. To prevent this you should only restart the server when there's no errors.

@gregmartyn
Copy link
Contributor

It's not working for me. What reliably breaks HMR for me is a syntax error. E.g. put [[[[[[[[[ somewhere in one of your files.

nodejs_1 | [7:23:02 PM] Compiling server
nodejs_1 | [7:23:02 PM] Compiled server in 178ms
nodejs_1 | [HMR] Cannot apply update.
nodejs_1 | [HMR] Error: Module build failed (from ./razzle/packages/razzle/node_modules/babel-loader/lib/index.js):
nodejs_1 | SyntaxError: Unexpected token (45:0)

After that, even with this patch, I'll see stuff like this in the log:
nodejs_1 | [7:23:18 PM] Compiling client
nodejs_1 | [7:23:18 PM] Compiling server
nodejs_1 | [7:23:18 PM] Compiled server in 477ms
nodejs_1 | [7:23:18 PM] Compiled client in 479ms
nodejs_1 | [7:23:19 PM] Compiling client
nodejs_1 | [7:23:19 PM] Compiled client in 671ms
nodejs_1 | [7:23:26 PM] Compiling server
nodejs_1 | [7:23:31 PM] Compiled server in 5s

but never
nodejs_1 | [HMR] Update applied.

@jaredpalmer
Copy link
Owner

@crosscompile does this happen to us too?

cc @TheLarkInn any ideas about what would break HMR ?

@Bram-Zijp
Copy link
Contributor Author

Bram-Zijp commented Aug 29, 2018

Note: This is for server sided errors only.
Try generate an error in the server.js file. With a fresh Razzle project it will log the [HMR] You need to restart the application! message

In the following stack trace I imported a test.js file in the server.js file. Then I added an undefined variable.

√ success server compiled in 270ms
✅  Server-side HMR Enabled!
🚀 started
√ success server compiled in 64ms
🔁  HMR Reloading `./server/server.js`...
[HMR] Cannot apply update.
[HMR] ReferenceError: asdfasdf is not defined
    at Module../src/server/test.js (A:\razzle-example\build\webpack:\src\server\test.js:1:1)
    at __webpack_require__ (A:\razzle-example\build\webpack:\webpack\bootstrap:682:1)
    at fn (A:\razzle-example\build\webpack:\webpack\bootstrap:59:1)
    at Module../src/server/server.js (A:\razzle-example\build\server.js:1097:63)
    at __webpack_require__ (A:\razzle-example\build\webpack:\webpack\bootstrap:682:1)
    at fn (A:\razzle-example\build\webpack:\webpack\bootstrap:59:1)
    at A:\razzle-example\build\webpack:\src\index.js:34:1
    at A:\razzle-example\build\webpack:\src\index.js:38:1
    at hotApply (A:\razzle-example\build\webpack:\webpack\bootstrap:590:1)
    at A:\razzle-example\build\webpack:\webpack\bootstrap:272:1
[HMR] You need to restart the application!

@gregmartyn
Copy link
Contributor

ReferenceError and SyntaxError both block HMR for me on the server-side, with and without this patch. I never end up in the catch()

@Bram-Zijp
Copy link
Contributor Author

My bad. I didn't include the whole solution. It should work now.

Copy link
Owner

@jaredpalmer jaredpalmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool

@gregmartyn
Copy link
Contributor

@jaredpalmer did this actually work for you? It didn't make a difference for me. HMR was erroring before it called module.hot.accept

@jaredpalmer
Copy link
Owner

@crosscompile is investigating this today.

@jackjocross
Copy link
Collaborator

This is working for me locally, nice stuff @Bram-Zijp!

@gregmartyn are you sure you have the entire patch? Initially I missed changing the server import to a require and for some reason that allows the server to reload.

let app = require('./server').default;

@gregmartyn
Copy link
Contributor

I did something similar. Tried again and it's working. Thanks!

@jackjocross jackjocross merged commit f15ccc1 into jaredpalmer:master Sep 27, 2018
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 this pull request may close these issues.

4 participants