Skip to content

resolveNetworkAddress: Listen for close instead of exit; Fix FailedApp theme#29046

Merged
ravicious merged 3 commits intomasterfrom
ravicious/node-stdout
Jul 13, 2023
Merged

resolveNetworkAddress: Listen for close instead of exit; Fix FailedApp theme#29046
ravicious merged 3 commits intomasterfrom
ravicious/node-stdout

Conversation

@ravicious
Copy link
Copy Markdown
Member

@ravicious ravicious commented Jul 13, 2023

We had another instance of a flaky resolveNetworkAddress test. We already tried to deal with this in the past (#25249), but we might have as well fixed one or zero problems at that time while there was another lurking under the surface.

resolveNetworkAddress receives a child process. It listens to both process.stdout.on('data') and process.on('exit'). resolveNetworkAddress resolves when the child process prints a specific string to stdout or rejects with an error if the process exits before printing the string.

The problem is that the fake process we use in tests exits almost immediately after printing since there's nothing else to do.

To guarantee that resolveNetworkAddress processes the data listener before the exit listener, we have to listen for close instead of exit as the docs say):

The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams. The 'close' event will always emit after 'exit' was already emitted, or 'error' if the child failed to spawn.


That's the original issue fixed by this PR. However, while testing the fix in dev version of Connect, by doing os.Exit right before the port is logged and making the app failed to start, I discovered another problem. The error component AKA FailedApp was not being rendered successfully.

Because of the theme changes, the Card component, used in FailedApp, now requires a theme to be rendered. I thought the issue was simply that Card.defaultProps were incorrect, but after fixing that I discovered that there's a slew of other components which also require a theme or typography but don't have those props set by default.

Instead of fixing them one by one, I decided to simply add a static theme provider so that those components can render. I picked the dark theme so that we don't flash people with a white screen in case of an error.

@ravicious ravicious changed the title resolveNetworkAddress: Listen for close instead of exit resolveNetworkAddress: Listen for close instead of exit; Fix FailedApp theme Jul 13, 2023
@github-actions github-actions Bot requested review from kimlisa and ryanclark July 13, 2023 09:20
@ravicious ravicious enabled auto-merge July 13, 2023 09:20
@ravicious ravicious requested review from gzdunek and removed request for kimlisa July 13, 2023 09:21
Copy link
Copy Markdown
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from gzdunek July 13, 2023 17:03
@ravicious ravicious added this pull request to the merge queue Jul 13, 2023
Merged via the queue into master with commit 5505511 Jul 13, 2023
@ravicious ravicious deleted the ravicious/node-stdout branch July 13, 2023 17:21
@public-teleport-github-review-bot
Copy link
Copy Markdown

@ravicious See the table below for backport results.

Branch Result
branch/v11 Failed
branch/v12 Failed
branch/v13 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants