Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused SIGINT listener #237

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

northword
Copy link
Contributor

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();
  };
}

northword added a commit to northword/zotero-plugin-scaffold that referenced this pull request Sep 1, 2024
@sindresorhus sindresorhus merged commit 4a62e50 into yeoman:main Sep 9, 2024
2 checks passed
@sindresorhus sindresorhus changed the title fix: do not listen SIGINT Remove unused SIGINT listener Sep 9, 2024
@northword northword deleted the disable-listening-sigint branch September 9, 2024 23:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants