Broken HMR via signal

[7rulnik]

Hello, thanks for this fork. We have been using it for years without any problems.

Recently, we decided to enable HMR in our project, and I realized that probably the current behavior is incorrect or could be enhanced at least.

I think that direction that was taken here #35 is the wrong one (I don't want to offend anybody, it's just my opinion that could be wrong too).

So in HMR there are 2 ways to apply an update to the process:

1. file system polling — webpack checks for new hot-update files on disk and triggers HMR
2. signal — something (run-script-webpack-plugin in our case) sends kill to process using SIGUSR2 (default one, could be configured) signal

After #35 we don't send kill anymore, which makes it impossible to use signal HMR API. Probably the correct way to solve it was filtering SIGUSR2 signal:

import { ShutdownSignal } from '@nestjs/common'

app.enableShutdownHooks(
  Object.values(ShutdownSignal).filter((signal) => signal !== 'SIGUSR2')
)

For fs polling current behaviour is works without any problem (I did not check it by myself but looking at code it should).

---

But I think the behaviour before this change wasn't the best either. Normally, we just kill and start process after emit which works fine. But in HMR we never actually kill process — just sending signal. The problem here is that HMR can corrupt or broke app state so the best way to recover from this — restart the app. But currently it's not possible without full restart of bundler build.

What I suggest: rs (restart via keyboard) should work the same way for normal and hmr setups — just restart the app completely.

Also, I want to mention that restartable, which hides rs feature not documented at all.

---

Summary:

1. HMR flow should be different for fs polling and signal API
2. rs should always restart the app completely

Currently, I patched this plugin to support signal API and rs but not fs polling:

import { ChildProcess, fork } from 'child_process';
import type { Compilation, Compiler, WebpackPluginInstance } from 'webpack';

export type RunScriptRspackPluginOptions = {
  cwd?: string;
  env?: NodeJS.ProcessEnv;
  args: string[];
  nodeArgs: string[];
  name?: string;
  hmr?: boolean;
  keyboardRestart?: boolean;
};

export class RunScriptRspackPlugin implements WebpackPluginInstance {
  private readonly options: RunScriptRspackPluginOptions;

  private childProcess?: ChildProcess;

  private entrypoint?: string;

  constructor(options: Partial<RunScriptRspackPluginOptions> = {}) {
    this.options = {
      ...options,
      args: options.args || [],
      nodeArgs: options.nodeArgs || process.execArgv,
    };

    this.enableKeyboardRestart();
  }

  private enableKeyboardRestart(): void {
    if (this.options.keyboardRestart) {
      process.stdin.setEncoding('utf8');
      process.stdin.on('data', (data: string) => {
        if (data.trim() === 'rs') {
          this.stopProcess();
          this.startProcess();
        }
      });
    }
  }

  private afterEmit = (compilation: Compilation): void => {
    this.setEntrpointPath(compilation);

    if (
      this.childProcess &&
      this.childProcess.connected &&
      this.childProcess?.pid
    ) {
      if (this.options.hmr) {
        this.sendHmrSignal();
      } else {
        this.stopProcess();
        this.startProcess();
      }
    } else {
      this.startProcess();
    }
  };

  apply = (compiler: Compiler): void => {
    compiler.hooks.afterEmit.tap({ name: 'RunScriptPlugin' }, this.afterEmit);
  };

  private setEntrpointPath(compilation: Compilation) {
    const { assets, compiler } = compilation;
    const { options } = this;
    let name;
    const names = Object.keys(assets);
    if (options.name) {
      name = options.name;
      if (!assets[name]) {
        // eslint-disable-next-line no-console
        console.error(
          `Entry ${name} not found. Try one of: ${names.join(' ')}`,
        );
      }
    } else {
      [name] = names;
      if (names.length > 1) {
        // eslint-disable-next-line no-console
        console.log(
          `More than one entry built, selected ${name}. All names: ${names.join(
            ' ',
          )}`,
        );
      }
    }
    if (!compiler.options.output || !compiler.options.output.path) {
      throw new Error('output.path should be defined in webpack config!');
    }

    this.entrypoint = `${compiler.options.output.path}/${name}`;
  }

  private startProcess(): void {
    if (!this.entrypoint)
      throw new Error('run-script-webpack-plugin requires an entrypoint.');

    const { args, nodeArgs, cwd, env } = this.options;
    this.childProcess = fork(this.entrypoint, args, {
      execArgv: nodeArgs,
      stdio: 'inherit',
      cwd,
      env,
    });
  }

  private stopProcess() {
    if (this.childProcess?.pid) {
      // eslint-disable-next-line no-console
      console.log(
        `Sending SIGTERM signal to process with ${this.childProcess.pid} pid`,
      );

      try {
        process.kill(this.childProcess.pid, 'SIGTERM');
      } catch (error) {
        // eslint-disable-next-line no-console
        console.error(
          `Failed to stop process with pid ${this.childProcess.pid}:`,
          error,
        );
      }
    }
  }

  private sendHmrSignal() {
    if (this.childProcess?.pid) {
      // eslint-disable-next-line no-console
      console.log(
        `Sending SIGUSR2 signal to process with ${this.childProcess.pid} pid`,
      );
      process.kill(this.childProcess.pid, 'SIGUSR2');
    }
  }
}

cc @sergiupris

Related to #21

[atassis]

Hi there. Thanks for the thorough analysis of what's happening in this repo, I'll definitely finish reviewing this issue during this week (including weekends). It raised a bunch of issues and questions already and has to be done correctly.

[atassis]

UPD: Due to some personal problems I lack time to address this issue properly, will take a bit more time.
I have checked the code already, good job sorting things out in a readable way.
Still, have to revisit all steps done for the last half a year so I don't break anything as we already did 🙂 .
Thinking about releasing alpha version with major bump due to new upcoming LTS, so we can combine all the changes.

[7rulnik]

No worries, feel free to ping me for testing alpha or if you need any help

[atassis]

It took me a while to handle some crap happening around me.
I am reviewing the repo again. A main issue for me here is that I have no projects around me to use it, so I decided to at least vibe code some tests to start with, and then review the suggested code and use it.
It will take a little while to check the tests I generated before introducing in the repo. After that I'll step by step review the current logic in the repo and urs suggested. I have already broken smth for people and don't wanna introduce any more mess.
So, for now, I'll maybe do this in two steps, in which I may use your help if that's ok: Introduce some tests in the PR, and after tests- another PR with some logic changes to the package.

[atassis]

And yeah, if you ever happen to be around North Ossetia - a beer from me!
Even though Valya doesn’t read any issues (but writes them haha), your past contributions are still welcome!