fix hmr for variable PORT

[gregmartyn]

Docker containers (and any 12 factor app) typically respond to environment variable changes at runtime, not at build time. If the DefinePlugin array is modified in razzle.config.js from the default that hardcodes HOST and PORT to treat them as regular variables instead, then they aren't available as process.env.HOST / PORT from client-side code. (though they are available in server-side code) It mostly works, but there's an issue with it:

This existing code is used to setup the port for WebpackDevServer for the HMR of the client build (the socket that handles browser hot reload, not the nodejs port):
port: parseInt(process.env.PORT, 10) + 1 || window.location.port,

1. If PORT is 3000, the first part correctly returns 3001. If the PORT env var isn't set, the window.location.port fallback is invoked, but that would return 3000. It's missing that +1. This is the main issue here.
2. window.location.port is "" if port is 80 (you can run the app on a different port, but it's also easy to handle gracefully so that there are no papercuts / surprises)

We can fix both of those without having any impact at all on existing users. Things would continue to work the same for them, but anyone who is turning HOST and PORT into true environment variables would benefit.

[jaredpalmer]

Why should 81 be the default?

[gregmartyn]

Another way to write it would've been:
parseInt(window.location.port || window.location.protocol === 'http:' ? 80 : 443, 10) + 1

Maybe that's clearer. window.location.port is "" if the port was inferred from the location's protocol.

We add 1 because the client build uses PORT+1

[gregmartyn]

@jaredpalmer are there any changes you'd like to see? questions? comments? thanks

[gregmartyn]

Is anything holding this back? It's a bugfix to the existing code
port: parseInt(process.env.PORT, 10) + 1 || window.location.port,
where the part after || needs the same + 1 that the first part has.

[gregmartyn]

@jaredpalmer updated the description. Does that help explain it?

[stale]

Hola! So here's the deal, between open source and my day job and life and what not, I have a lot to manage, so I use a GitHub bot to automate a few things here and there. This particular GitHub bot is going to mark this as stale because it has not had recent activity for a while. It will be closed if no further activity occurs in a few days. Do not take this personally--seriously--this is a completely automated action. If this is a mistake, just make a comment, DM me, send a carrier pidgeon, or a smoke signal.

[stale]

ProBot automatically closed this due to inactivity. Holler if this is a mistake, and we'll re-open it.

[gregmartyn]

@jaredpalmer hello?

[gregmartyn]

@jaredpalmer this isn't resolved by #761

#761 is about where the browser connects
#581 (this) is about where the server listens

[krazyjakee]

@gregmartyn could this be related? (afterjs uses razzle) jaredpalmer/after.js#183

[gregmartyn]

@jaredpalmer can this be considered for v3?

[gregmartyn]

@jaredpalmer squeaky wheel here. Would you be up for a phone call to discuss this?

[gregmartyn]

@jaredpalmer again, if you have any questions, concerns, or comments about this, please let met know.

[gregmartyn]

v3 version is #1020

[elliottjro]

hi @gregmartyn sorry for the super late reply. Is this PR still related or has this been fixed?
If still related, could you please fix the conflicts and I will take another look.

Thanks for the patience!

[fivethreeo]

Has not been fixed. The fix is incomplete and better solved by upgrading webpack-dev-server.

[elliottjro]

if this is better solved by upgrading webpack-dev-server, I will close this PR. @fivethreeo pls confirm

[gregmartyn]

I still need this fix. We're doing docker builds, where PORT isn't being hardwired into the image. process.env.PORT is available on the server-side, but not client-side, so it falls back to using window.location.port. That'd be fine except window.location.port is not set if you're using port 80 or 443.

[gregmartyn]

@fivethreeo what's the webpack-dev-server change you were referring to? Is there a PR for it? This fix at least lets me work with the current state of things..

[fivethreeo]

Check out razzle-plugin-proxy for a temporary fix.

[fivethreeo]

No PR since we probably need to upgrade webpack aswell.

[fivethreeo]

Oh, merged :) NP. But still need to fix this using new webpack-dev-server.

[elliottjro]

yeaa if you make a new PR with webpack updates I can prioritize it

[fivethreeo]

Will give it a try.

[gregmartyn]

razzle-plugin-proxy is a bit extreme. A whole reverse proxy? What does it solve now that #581 is merged?

[fivethreeo]

Solves proxying to a subpath on the domain. Also having express and webpack-dev-server on the same port so you can show it to customers without messing with extra port forwarding. But with the new webpack-dev-server this works automatically since it detects where it is served from.

[thecodedrift]

Just a quick list of fixes I discovered when working on this problem. For context, I needed CLIENT_PUBLIC_PATH to be an emulated CDN with ssl support running on 443. To make it work, I made the following changes.

sockJsPort in webpackHotDevClient

Current behavior: We use the serverPort and assume (even when CLIENT_PUBLIC_PATH is defined) that the socket server must be on serverPort + 1. We fixed the serverPort with #581 when the server is on port 80/443, but that doesn't do much for the socket server / client webpack build.

Suggested Change: Since we're already importing url we can just parse and check the port of CLIENT_PUBLIC_PATH if its provided. Alternatively, we could add more env vars such as SOCK_PORT and SOCK_HOST, but with the way razzle is set up, our socket server is currently always on the public path location.

var sockJsPort = serverPort + 1;
var cppParts = {};

if (process.env.CLIENT_PUBLIC_PATH) {
  cppParts = url.parse(process.env.CLIENT_PUBLIC_PATH);
  if (cppParts.port) {
    sockJsPort = cppParts.port;
  } else {
    sockJsPort = cppParts.protocol === "https:" ? 443 : 80;
  }
}

Hotfix: This can be hotfixed by making your own copy of razzle-dev-utils/webpackHotDevClient with the above change and then replacing the file path in config.entry.client.

const { resolve } = require("path");

module.exports = {
  modify: (config, { target, dev }, webpack) => {
    if (dev && config.entry.client) {
      config.entry.client.forEach((f, i) => {
        if (f.indexOf("webpackHotDevClient") >= 0) {
          const patch = resolve(__dirname, "./razzle_custom/webpackHotDevClient.js");
          config.entry.client[i] = patch;
        }
      });
    }
    return config;
  }
}

Upgrading webpack-dev-server

Current behavior: We are using an older webpack-dev-server.

Suggested Change: As of 3.4.0, WDS has the sockPort and sockHost variables. Those can be used in the same way as above, utilizing url and matching both socket and WDS values.

const url = require(url);

// ...
// later on when we define the devServer's port
// ...

let devServerPort = parseInt(dotenv.raw.PORT, 10) + 1;
let devServerHost = "localhost";
// VMs, Docker containers might not be available at localhost:3001. CLIENT_PUBLIC_PATH can override.
const clientPublicPath =
    dotenv.raw.CLIENT_PUBLIC_PATH ||
    (IS_DEV ? `http://${dotenv.raw.HOST}:${devServerPort}/` : '/');

// if CLIENT_PUBLIC_PATH is set, select new devServer host/port values
if (dotenv.raw.CLIENT_PUBLIC_PATH) {
  const parts = url.parse(dotenv.raw.CLIENT_PUBLIC_PATH);
  devServerPort = parts.port || (parts.protocol === "https") ? 443 : 80;
  devServerHost = parts.hostname;
}

// ...
// still later on when we define the devServer config
// ...

config.devServer = {
  // ...
  port: devServerPort,
  host: devServerHost,
  sockPort: devServerPort,
  sockHost: devServerHost,
  // ...
}

Hotfix: Hotfixing this inside of a razzle.config.js requires you to (1) install an upgraded webpack-dev-server and use yarn's resolvers to run WDS >= 3.4.0, then (2) do a similar interpretation of the CLIENT_PUBLIC_PATH like we did for the webpackHotDevClient above.

const { resolve } = require("path");
const url = require("url");

module.exports = {
  modify: (config, { target, dev }, webpack) => {
    // enable new sockPort and sockHost functionality
    // req yarn to resolve webpack-dev-server to 3.4.0+
    if (dev && config.devServer && process.env.CLIENT_PUBLIC_PATH) {
      let port = (process.env.PORT || 3000) + 1;
      let host = "localhost";
      const cppParts = url.parse(process.env.CLIENT_PUBLIC_PATH);
      host = cppParts.hostname;
      if (cppParts.port) {
        port = cppParts.port;
      } else {
        port = cppParts.protocol === "https:" ? 443 : 80;
      }

      config.devServer.host = host;
      config.devServer.port = port;
      config.devServer.sockHost = host;
      config.devServer.sockPort = port;
    }
    return config;
  }
}

These are highly experimental, YMMV. I'm happy to wrap all this up into a PR if there's interest. I doubt we're the only ones over here doing docker+razzle with some SSL in the mix.

[mattlubner]

Maybe I've misunderstood the issue here, but what's gained from applying the 12-factor methodology to running Razzle locally in development mode? Razzle is already 12-factor in production mode with minimal changes.

FWIW, we also use Docker with Razzle (locally & deployed), and our solution was to leave the development ports at their default values (3000 & 3001), map both ports from the Docker container to localhost, and then use nginx-proxy + dnsmasq to access the Razzle site locally via an adhoc hostname (eg. http://example.razzle.localhost). With a self-signed SSL certificate, https: will work as well. HRM also works, since port 3001 on the fake hostname still resolves to localhost:3001, which gets mapped to the container.

[thecodedrift]

@mattlubner the problem crops up when CLIENT_PUBLIC_PATH isn't using port 3001 for whatever reason. At that point, razzle can't find the WDS, and you lose HMR. It's not the end of the world, but it is solved by having sockPort/host be pulled from the public path var if provided.