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

refactor(core): allow restart logic to be triggered using an event #3380

Closed
wants to merge 2 commits into from

Conversation

erikian
Copy link
Member

@erikian erikian commented Oct 13, 2023

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:
This allows the restart event to be triggered from anywhere in Forge and should make it possible for @caoxiemeihao to address the process.stdin.emit issue mentioned in #3203 (comment):

Ideally, we should instead expose additional start logic from the @electron-forge/core API to expose this functionality so that plugins can hook into it.

At first, I was thinking of creating a dedicated EventEmitter instance for this, but process is an event emitter and is available everywhere, so that makes things simpler if a bit hacky.

@erikian erikian requested a review from a team as a code owner October 13, 2023 01:34
Copy link
Member

@BlackHole1 BlackHole1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. PTAL @caoxiemeihao

Copy link
Member

@caoxiemeihao caoxiemeihao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! 👍

@erikian erikian requested a review from a team October 13, 2023 03:05
Copy link
Member

@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still feels side-effecty/hacky. Like, we're still emitting a random string on a random emitter. This should just be a method exposed via forge core, or plugin base that can be called normally

@erikian
Copy link
Member Author

erikian commented Oct 13, 2023

we're still emitting a random string on a random emitter

What about:

// at the top-level of the module
const restartAppEventEmitter = new EventEmitter()

// ...
if (interactive) {
  restartAppEventEmitter.on('restart', async () => {
    if (lastSpawned) {
      console.info(chalk.cyan('\nRestarting App\n'));
      lastSpawned.restarted = true;
      lastSpawned.kill('SIGTERM');
      lastSpawned.emit('restarted', await forgeSpawnWrapper());
    }
  });

  process.stdin.on('data', async (data) => {
    if (data.toString().trim() === 'rs') {
      restartApp();
    }
  });

  process.stdin.resume();
}
// ...

export function restartApp() {
  restartAppEventEmitter.emit('restart');
}

This would work without consumers directly emitting anything and still allows us to keep the logic simple.

@erikian
Copy link
Member Author

erikian commented Nov 30, 2023

Here's the alternative I mentioned in my previous comment: 3a3cf97. I believe using some form of event-based approach is the best option here — the restart logic depends on variables that are inside the scope of the enclosing start function, and exposing a restartApp method without using events would involve declaring those variables at the top-level of the module and then assigning them from the start function so restartApp can also access them, which is much uglier IMO.

@erikian erikian force-pushed the refactor/restart-event branch 2 times, most recently from 6acb75a to 3a3cf97 Compare November 30, 2023 17:06
@caoxiemeihao
Copy link
Member

Implemented in #3468

@caoxiemeihao caoxiemeihao deleted the refactor/restart-event branch February 9, 2024 07:44
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.

4 participants