atassis
Broken HMR via signal
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 https://github.com/atassis/run-script-webpack-plugin/pull/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:
- file system polling — webpack checks for new hot-update files on disk and triggers HMR
- signal — something (run-script-webpack-plugin in our case) sends kill to process using SIGUSR2 (default one, could be configured) signal
After https://github.com/atassis/run-script-webpack-plugin/pull/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:
- HMR flow should be different for fs polling and signal API
-
rsshould 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 https://github.com/atassis/run-script-webpack-plugin/issues/21
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.
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.