ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.69k stars 243 forks source link

Come up with better strategy for hot reloading of server #330

Open bkniffler opened 7 years ago

bkniffler commented 7 years ago

Hey, If disableSSR = true, changing code will still trigger a hot reload and server restart, since it will only affect the express servers response, not the server bundle building.

Is this the desired behavior?

smirea commented 7 years ago

ping on this reloading the server is causing issues in dev mode as the client sometimes resends requests to the server while it's being reloaded. There should be a mode to not hot reload the server with SSR disabled

EDIT: If anyone is having the same issue as I am with requests being sent while the dev server is hot reloading, I hacked it for now by adding a timeout to the client server. It ain't pretty, but gets the job done. @ctrlplusb plz halp

    // 👉  tools/development/hotNodeServer.js:57

    const waitForClientThenStartServer = () => {
      if (this.serverCompiling) {
        // A new server bundle is building, break this loop.
        return;
      }
      if (this.clientCompiling) {
        setTimeout(waitForClientThenStartServer, 50);
      } else {
        // 👉  TODO: HACK: remove this timeout when you can disable server hot reloading
        setTimeout(() => startServer(), 200);
      }
    };
bkniffler commented 7 years ago

Cool @smirea, I'm suffering similar issues, assets and data served by the express server will not load properly for the reason you stated. Your solution might work to get around this for now, but having the possibility to not restart the whole server if something in your react app changed while SSR is disabled would be very nice.

ctrlplusb commented 7 years ago

Agreed

ctrlplusb commented 7 years ago

I did look at this and it turned into a bigger pain than anticipated. I am going to give the tooling a once over for v13 and hopefully I can come up with a more elegant solution.

ctrlplusb commented 7 years ago

Finally have a solution for this. 😀

It's perhaps not a perfect solution, however, I think it will acceptable. Basically, when ssr is disabled I configure the server's webpack watcher to ignore any paths in the shared folder. So the server will only rebuild for changes within it's own folder.

One case this would fail though: if a change is made to some shared code that isn't related to the frontend only (e.g. a util). This won't cause the server to reload. What are your guys thoughts on how to handle this case? We could beef up the CLI output a bit and allow users to manually trigger a server restart? Or somehow make the ignore flag a bit more clever?

This will be going into the next branch.

Here is a code snippet update on development/hotNodeServer.js:

    // Lets start the compiler.
    this.watcher = compiler.watch(
      {
        ignored: config('disableSSR')
          ? `${path.resolve(appRootDir.get(), config('sharedCodePath'))}/**/*`
          : undefined,
      },
      () => undefined,
    )
ctrlplusb commented 7 years ago

Perhaps something like this could make it a bit more intelligent?

https://github.com/dependents/node-dependency-tree

I just worry the additional complexity introduces flakiness.

birkir commented 7 years ago

Good work there @ctrlplusb

So now we will have to stop / start the dev server if we change that disableSSR flag, right?

ctrlplusb commented 7 years ago

@birkir

Yeah, but only for changes to the shared folder(s). Any change to the server folder itself would cause it to restart automatically.

I have been trying out this solution in a personal project of mine, to see if it is too confusing or not. We will have to output more appropriate messages to let the user know what is going on too, and ask them if they would like to restart the server. May need to buff up the the CLI interface/output. There are some cool tools for this, e.g. https://github.com/mattallty/Caporal.js

ctrlplusb commented 7 years ago

Thought of another cool possibility...

Only auto hot-reload client - however, when server changes send a message to the client. The client then displays a toast/notification informing them that the server code has changed, and that they can click the notification to restart the server whenever they would like. Avoids user having to switch back out to the console to do a manual restart and allows you a grace period where hot reloading the client with components hitting server APIs etc don't fall over.

Thoughts?

birkir commented 7 years ago

I think I would use that feature very often. Lots of time I hot reload and the server takes too long to restart so when the client reloads and needs some local api endpoint, its not available.

strues commented 7 years ago

I've been thinking about a solution to this issue for a while now. Rather than creating the server bundle during development, what about running the server as is. By reconfiguring the server entry point and all of the development scripts this could work.

I dont have much time so pardon the pseudo-code

  1. yarn develop => babel-node server/index.js
  2. logic in index.js pulls in our listener & express app() from say, app.js
  3. index.js imports our hotreload and passes it the app instance
  4. Hot reloading files condensed below:
 // basically what we have now, but additionally including server
const serverConfigFiles = [ /\/server\//, /\/config\//, /\/internal\// ]

// client/shared files in addition to server/config
const allWatchFiles = [ /\/client\//, /\/shared\//, ...serverConfigFiles ]

// regex to match I'd use Ramda, but maybe someone 
// smarter (not as lazy) could write it (R.any) and we wouldnt need to add ramda
const fileMatch = (regexs, id) => R.any((regex) => regex.test(id))(regexs)

const compilerPlugin = webpack(config)
const watcher = chokidar.watch(SERVER PATH aka server/**)

compiler.plugin('compile', () => log...)
compiler.plugin('compilation', () => log...)

// add webpack-dev-mw  & pass it compiler
app.use( webpack-dev-mw(compiler, {devMWoptions })

watcher.on('ready', () => {
    watcher.on('all', (event, file) => {
      Object.keys(require.cache).forEach((id) => {
        if (fileMatch(serverConfigFiles, id)) {
          delete require.cache[id]
        }
      })
    })
  })
  compiler.plugin('done', () => {
    Object.keys(require.cache).forEach((id) => {
      if (fileMatch(allWatchFiles, id)) {
        delete require.cache[id]
      }
    })
  })

Also looking at the Webpack-Dev-Middleware docs, it might be worthwhile to experiment with the SSR features, see the bottom of their docs

peshi commented 7 years ago

I am using this in my current project, https://github.com/60frames/webpack-hot-server-middleware

birkir commented 7 years ago

@peshi implementation details? It's pretty hard to use with react-universally because of the split server/client thing.

peshi commented 7 years ago

@birkir Well, I came across this project (react-universally) yesterday. I started today and spent a couple of hours to rip it apart and managed to get and compile both configs in memory. But there's still a lot of magic and code to throw in the trashcan. I might re-visit that code when I have more free time.

birkir commented 7 years ago

Cool, thanks. I got it working, but it was always reloading my server. Still need to work on it.

ctrlplusb commented 7 years ago

All, I have a new effort I am building on the side. It includes a significantly simplified (and enhanced) developer toolchain. I hope to have some prototypes up soon, after which we could look at migrating it into the project.

The webpack-hot-server-middleware looks interesting too!

Aetherall commented 6 years ago

Actually I was using webpack-hot-server-middleware before switching over this boilerplate because errors are swallowed by the webpack plugin hook

Maybe this is misconfiguration though, but take care of checking how errors behave if you try to implement that

mschipperheyn commented 6 years ago

Any news on this? In my case, I've got quite a bit going on the server, making shared code changes a slow process

grahamd711 commented 5 years ago

I've recently had the below issue when the dev server reloads due to a minor change. Thinking it is related to the new bundle being built. I'm returning data on http://localhost:7331/__webpack_hmr so I was not sure if this was related. Anyone else experiencing similar issues?

SERVER: Error in server execution, check the console for more info. Error: listen EADDRINUSE :::3000

@birkir did you run into any issues like this when setting .env variables for the mP site?

oyeanuj commented 5 years ago

@grahamd711 Also, seeing this suddenly. Did you find a fix for this?

grahamd711 commented 5 years ago

@oyeanuj , yes still occurring on my end. shutting down the server and running yarn dev again works, but the hot reloading is still an issue. Any luck on your side?

oyeanuj commented 5 years ago

@grahamd711 None yet :( I upgraded RHL, Webpack, etc but no luck yet. Searching online it suggested that it might have something to do with nodemon but that isn't something that I've installed.