-
Notifications
You must be signed in to change notification settings - Fork 240
use central unmount for ink render tree; stop messing with node's event loop #7153
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,49 @@ import {Logger, LogLevel} from '../../public/node/output.js' | |
| import {isUnitTest} from '../../public/node/context/local.js' | ||
| import {treeKill} from '../../public/node/tree-kill.js' | ||
|
|
||
| import {ReactElement} from 'react' | ||
| import {Key, render as inkRender, RenderOptions} from 'ink' | ||
| import React, {ReactElement, createContext, useCallback, useContext, useEffect, useState} from 'react' | ||
| import {Key, render as inkRender, RenderOptions, useApp} from 'ink' | ||
|
|
||
| import {EventEmitter} from 'events' | ||
|
|
||
| const CompletionContext = createContext<((error?: Error) => void) | null>(null) | ||
|
|
||
| /** | ||
| * Signal that the current Ink tree is done. Must be called within an | ||
| * InkLifecycleRoot — throws if the provider is missing so lifecycle | ||
| * bugs surface immediately instead of silently hanging. | ||
| */ | ||
| export function useComplete(): (error?: Error) => void { | ||
| const complete = useContext(CompletionContext) | ||
| if (!complete) { | ||
| throw new Error('useComplete() called outside InkLifecycleRoot') | ||
| } | ||
| return complete | ||
| } | ||
|
|
||
| /** | ||
| * Root wrapper for Ink trees. Owns the single `exit()` call site — children | ||
ryancbahan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| * signal completion via `useComplete()`, which sets state here. The `useEffect` | ||
| * fires post-render, guaranteeing all batched state updates have been flushed | ||
| * before the tree is torn down. | ||
| */ | ||
| export function InkLifecycleRoot({children}: {children: React.ReactNode}) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i don't know ink that well, but i am hoping there's a better way to do this. having to manually manage teardown |
||
| const {exit} = useApp() | ||
| const [exitResult, setExitResult] = useState<{error?: Error} | null>(null) | ||
|
|
||
| const complete = useCallback((error?: Error) => { | ||
| setExitResult({error}) | ||
| }, []) | ||
|
|
||
| useEffect(() => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this still feels incredibly cursed. the ink docs mention it's a common case, but i want to keep thinking about this (or remove ink). imperatively managing unmount is exactly the thing i don't want to do if i have react. but at least for now it removes the mnual event loop ticks and race conditions |
||
| if (exitResult !== null) { | ||
| exit(exitResult.error) | ||
| } | ||
| }, [exitResult, exit]) | ||
|
|
||
| return <CompletionContext.Provider value={complete}>{children}</CompletionContext.Provider> | ||
| } | ||
dmerand marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| interface RenderOnceOptions { | ||
| logLevel?: LogLevel | ||
| logger?: Logger | ||
|
|
@@ -27,10 +65,8 @@ export function renderOnce(element: JSX.Element, {logLevel = 'info', renderOptio | |
| } | ||
|
|
||
| export async function render(element: JSX.Element, options?: RenderOptions) { | ||
| const {waitUntilExit} = inkRender(element, options) | ||
| const {waitUntilExit} = inkRender(<InkLifecycleRoot>{element}</InkLifecycleRoot>, options) | ||
| await waitUntilExit() | ||
| // We need to wait for other pending tasks -- unmounting of the ink component -- to complete | ||
| return new Promise((resolve) => setImmediate(resolve)) | ||
| } | ||
ryancbahan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| interface Instance { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,7 @@ describe('ConcurrentOutput', () => { | |
| // Given | ||
| const backendSync = new Synchronizer() | ||
| const frontendSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this needs a better solution. these tests are already relying on promise flushing in a roundabout way. we should rethink the entire approach. |
||
|
|
||
| const backendProcess = { | ||
| prefix: 'backend', | ||
|
|
@@ -37,6 +38,7 @@ describe('ConcurrentOutput', () => { | |
| stdout.write('third backend message') | ||
|
|
||
| backendSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -50,6 +52,7 @@ describe('ConcurrentOutput', () => { | |
| stdout.write('third frontend message') | ||
|
|
||
| frontendSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| } | ||
| // When | ||
|
|
@@ -72,19 +75,23 @@ describe('ConcurrentOutput', () => { | |
| 00:00:00 │ frontend │ third frontend message | ||
| " | ||
| `) | ||
|
|
||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('strips ansi codes from the output by default', async () => { | ||
| const output = 'foo' | ||
|
|
||
| // Given | ||
| const processSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
| const processes = [ | ||
| { | ||
| prefix: '1', | ||
| action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { | ||
| stdout.write(`\u001b[32m${output}\u001b[39m`) | ||
| processSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| ] | ||
|
|
@@ -98,13 +105,15 @@ describe('ConcurrentOutput', () => { | |
| const logColumns = renderInstance.lastFrame()!.split('│') | ||
| expect(logColumns.length).toBe(3) | ||
| expect(logColumns[2]?.trim()).toEqual(output) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('does not strip ansi codes from the output when stripAnsi is false', async () => { | ||
| const output = '\u001b[32mfoo\u001b[39m' | ||
|
|
||
| // Given | ||
| const processSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
| const processes = [ | ||
| { | ||
| prefix: '1', | ||
|
|
@@ -113,6 +122,7 @@ describe('ConcurrentOutput', () => { | |
| stdout.write(output) | ||
| }) | ||
| processSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| ] | ||
|
|
@@ -126,11 +136,13 @@ describe('ConcurrentOutput', () => { | |
| const logColumns = renderInstance.lastFrame()!.split('│') | ||
| expect(logColumns.length).toBe(3) | ||
| expect(logColumns[2]?.trim()).toEqual(output) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('renders custom prefixes on log lines', async () => { | ||
| // Given | ||
| const processSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
| const extensionName = 'my-extension' | ||
| const processes = [ | ||
| { | ||
|
|
@@ -140,6 +152,7 @@ describe('ConcurrentOutput', () => { | |
| stdout.write('foo bar') | ||
| }) | ||
| processSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| ] | ||
|
|
@@ -161,12 +174,14 @@ describe('ConcurrentOutput', () => { | |
| const logColumns = unstyled(renderInstance.lastFrame()!).split('│') | ||
| expect(logColumns.length).toBe(3) | ||
| expect(logColumns[1]?.trim()).toEqual(extensionName) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('renders prefix column width based on prefixColumnSize', async () => { | ||
| // Given | ||
| const processSync1 = new Synchronizer() | ||
| const processSync2 = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
|
|
||
| const columnSize = 5 | ||
| const processes = [ | ||
|
|
@@ -175,13 +190,15 @@ describe('ConcurrentOutput', () => { | |
| action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { | ||
| stdout.write('foo') | ||
| processSync1.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| { | ||
| prefix: '1', | ||
| action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { | ||
| stdout.write('bar') | ||
| processSync2.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| ] | ||
|
|
@@ -206,22 +223,25 @@ describe('ConcurrentOutput', () => { | |
| // Including spacing | ||
| expect(logColumns[1]?.length).toBe(columnSize + 2) | ||
| }) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('renders prefix column width based on processes by default', async () => { | ||
| // Given | ||
| const processSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
| const processes = [ | ||
| { | ||
| prefix: '1', | ||
| action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { | ||
| stdout.write('foo') | ||
| processSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| {prefix: '12', action: async () => {}}, | ||
| {prefix: '123', action: async () => {}}, | ||
| {prefix: '1234', action: async () => {}}, | ||
| {prefix: '12', action: async () => gate.promise}, | ||
| {prefix: '123', action: async () => gate.promise}, | ||
| {prefix: '1234', action: async () => gate.promise}, | ||
| ] | ||
|
|
||
| // When | ||
|
|
@@ -234,20 +254,23 @@ describe('ConcurrentOutput', () => { | |
| expect(logColumns.length).toBe(3) | ||
| // 4 is largest prefix, plus spacing | ||
| expect(logColumns[1]?.length).toBe(4 + 2) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('does not render prefix column larger than max', async () => { | ||
| // Given | ||
| const processSync = new Synchronizer() | ||
| const gate = new Synchronizer() | ||
| const processes = [ | ||
| { | ||
| prefix: '1', | ||
| action: async (stdout: Writable, _stderr: Writable, _signal: AbortSignal) => { | ||
| stdout.write('foo') | ||
| processSync.resolve() | ||
| await gate.promise | ||
| }, | ||
| }, | ||
| {prefix: new Array(26).join('0'), action: async () => {}}, | ||
| {prefix: new Array(26).join('0'), action: async () => gate.promise}, | ||
| ] | ||
|
|
||
| // When | ||
|
|
@@ -260,6 +283,7 @@ describe('ConcurrentOutput', () => { | |
| expect(logColumns.length).toBe(3) | ||
| // 25 is largest column allowed, plus spacing | ||
| expect(logColumns[1]?.length).toBe(25 + 2) | ||
| gate.resolve() | ||
| }) | ||
|
|
||
| test('rejects with the error thrown inside one of the processes', async () => { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will stabilize theme app timeouts. previously only part of the render flow was mocked, but ink/react isn't fully set up in the test env, so we were getting timeouts. making renderTasks async mimics the existing concurrentoutput/etc behavior.