fix(cli): Prevent unwanted SIGINT error message from storybook #8977
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@Josh-Walker-GM working great on my mac. Will test on Windows tomorrow. Working great on Windows for me. |
jtoar
pushed a commit
that referenced
this pull request
Jul 27, 2023
**Problem** Storybook is showing an error message when you exit via a SIGINT. We have spent a few hours trying to understand why and work around this with no success. *Changes** 1. Conditionally disable the additional default signal listeners based on the argv contents. 2. Emit a custom event on `process` to trigger the telemetry shutdown. **Notes** I'm not really proud of this PR. Disabling based on the contents of argv isn't all that robust against false positives. Emitting a custom event on `process` is not really supposed to be done - hence why TS is complaining about the name of the signal. This work has also highlighted that on my windows 11 machine the background scripts are not firing... This was working when I updated the span values to match those found to work in this PR comment nodejs/node#21825 (comment) when I reenabled windows telemetry. Maybe something has changed, maybe it's version specific (os or node). This is an active nodejs issue so my ability to solve this in redwood code is understandably limited. Why isn't our CI finding this? We should investigate this too. @jtoar Could you confirm on mac and windows that this PR prevents those additional error messages from storybook. If you also have the time I'd appreciate if you could check if the telemetry background job is firing when you test on windows. I'll test this on ubuntu locally.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Storybook is showing an error message when you exit via a SIGINT.
We have spent a few hours trying to understand why and work around this with no success.
Changes*
processto trigger the telemetry shutdown.Notes
I'm not really proud of this PR. Disabling based on the contents of argv isn't all that robust against false positives. Emitting a custom event on
processis not really supposed to be done - hence why TS is complaining about the name of the signal.This work has also highlighted that on my windows 11 machine the background scripts are not firing... This was working when I updated the span values to match those found to work in this PR comment nodejs/node#21825 (comment) when I reenabled windows telemetry. Maybe something has changed, maybe it's version specific (os or node). This is an active nodejs issue so my ability to solve this in redwood code is understandably limited.
Why isn't our CI finding this? We should investigate this too.
@jtoar Could you confirm on mac and windows that this PR prevents those additional error messages from storybook. If you also have the time I'd appreciate if you could check if the telemetry background job is firing when you test on windows.
I'll test this on ubuntu locally.