yeoman / update-notifier

Update notifications for your CLI app
BSD 2-Clause "Simplified" License
1.76k stars 132 forks source link

Remove unused SIGINT listener #237

Closed northword closed 1 week ago

northword commented 2 weeks ago

ref: #98 ref: #75

Hello and thank you for your work!

I noticed that this project implemented a feature in #75 that show a update message when a SIGINT signal is emitted, however, in #98, the update message is removed and the process is exited.

Then there is no longer any need to register a SIGINT listener, and a SIGINT listener registered here would prevent the user from registering a SIGINT listener of their own.

For example:

In my project, I registered my own SIGINT listener in Serve to be able to kill script-started processes when Ctrl+C is pressed. However, since update-notifier has already registered the SIGINT listener and update-notifier always runs before my code, my SIGINT listener never takes effect unless I use process.removeAllListeners(“SIGINT”);. This is very inelegant, and I don't think update-notifier does anything substantial when SIGINT, so this listener can be removed.

// cli.ts this is bin

#!/usr/bin/env node

import { env, exit } from "node:process";
import { Command } from "@commander-js/extra-typings";
import updateNotifier from "update-notifier";
import { name, version } from "../package.json";
import { Log } from "./utils/log.js";
import { Build, Config, Release, Serve } from "./index.js";

const logger = new Log();

export default async function main() {
  updateNotifier({ pkg: { name, version } }).notify();

  const cli = new Command();
  cli
    .command("serve")
    .alias("dev")
    .description("Start development server")
    .action((_options) => {
      Config.loadConfig({}).then(ctx => new Serve(ctx).run());
    });

  cli.parse();
}

main().catch((err) => {
  logger.error(err);
  exit(1);
});
// serve.ts

...
export default class Serve extends Base {
  private builder: Build;
  private runner?: ServeBase;
  constructor(ctx: Context) {
    super(ctx);
    process.env.NODE_ENV ??= "development";
    this.builder = new Build(ctx);
  }

  async run() {
    // process.removeAllListeners("SIGINT");
    // Handle interrupt signal (Ctrl+C) to gracefully terminate Zotero process
    process.on("SIGINT", this.exit.bind(this));

    await this.ctx.hooks.callHook("serve:init", this.ctx);

    await this.builder.run();
    ...
  }

  exit = () => {
    this.logger.info("Server shutdown by user request.");
    this.runner?.exit();
    // Sometimes `runner.exit()` cannot kill the Zotero,
    // so we force kill it.
    killZotero();
    this.ctx.hooks.callHook("serve:exit", this.ctx);
    process.exit();
  };
}