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.

Sign up for GitHub

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

Jump to bottom

Fix waitUntilExit() not resolved with sync errors #563

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix waitUntilExit() not resolved with sync errors #563

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
585c784
fix: try to reproduce in CI
fantua Mar 18, 2023
dc8ad05
fix: waitUntilExit
fantua Mar 18, 2023
a893c6f
Merge branch 'master' into patch-1
fantua Apr 23, 2023
3ff8e7c
cr changes
fantua Apr 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/ink.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export default class Ink {
// This variable is used only in debug mode to store full static output
// so that it's rerendered every time, not just new static parts, like in non-debug mode
private fullStaticOutput: string;
private exitPromise?: Promise<void>;
private readonly exitPromise: Promise<void>;
private restoreConsole?: () => void;
private readonly unsubscribeResize?: () => void;

Expand Down Expand Up @@ -116,6 +116,11 @@ export default class Ink {
options.stdout.off('resize', this.resized);
};
}

this.exitPromise = new Promise((resolve, reject) => {
this.resolveExitPromise = resolve;
this.rejectExitPromise = reject;
});
}

resized = () => {
Expand Down Expand Up @@ -293,13 +298,6 @@ export default class Ink {
}

async waitUntilExit(): Promise<void> {
if (!this.exitPromise) {
this.exitPromise = new Promise((resolve, reject) => {
this.resolveExitPromise = resolve;
this.rejectExitPromise = reject;
});
}

return this.exitPromise;
}

Expand Down
8 changes: 5 additions & 3 deletions test/errors.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ test.after(() => {
restore();
});

test('catch and display error', t => {
test('catch and display error', async t => {
const stdout = createStdout();

const Test = () => {
throw new Error('Oh no');
};

render(<Test />, {stdout});
const {waitUntilExit} = render(<Test />, {stdout});

t.deepEqual(
stripAnsi((stdout.write as any).lastCall.args[0])
Expand All @@ -40,9 +40,11 @@ test('catch and display error', t => {
" 22: throw new Error('Oh no');",
' 23: };',
' 24:',
' 25: render(<Test />, {stdout});',
' 25: const {waitUntilExit} = render(<Test />, {stdout});',
'',
' - Test (test/errors.tsx:22:9)'
]
);

await t.throwsAsync(waitUntilExit, {message: 'Oh no'});
Copy link
Author

Choose a reason for hiding this comment

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

fix for Unhandled rejection in test/errors.tsx

Copy link
Owner

Choose a reason for hiding this comment

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

Unhandled rejection is getting thrown because exitPromise is being rejected when waitUntilExit hasn't been called, so no one is handling failure of exitPromise.

I like the solution of initializing exitPromise in the Ink constructor, but with the unhandled rejection it's a breaking change now.

To avoid it, we should add something like a isWaitingForExit boolean property to Ink and set it to true in waitUntilExit method. Then check that property in unmount and if no one is waiting for that promise to resolve or reject, don't call rejectExitPromise or resolveExitPromise.

That way we can catch synchronous errors without introducing breaking changes.

How does that sound?

Copy link
Author

Choose a reason for hiding this comment

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

To avoid it, we should add something like a isWaitingForExit boolean property to Ink and set it to true in waitUntilExit method.

With synchronous errors waitUntilExit is never called. I can add some prop to render method maybe. Wdyt?

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, wasn't that the purpose of this PR? To resolve/reject the promise returned by waitUntilExit for synchronous errors?

Copy link
Author

@fantua fantua Mar 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I'm saying that the proposed solution won't work. Let’s go over this pseudo-code and try to execute it:

const App = () => {
  throw new Error('');
  return null;
}

const { waitUntilExit } = render(<App />);
	
await waitUntilExit();
  1. Execute render()
    1. Set isWaitingForExit to false
    2. Throw error
    3. Call unmout with error
    4. Don't call rejectExitPromise becuase isWaitingForExit == false
  2. Execute waitUntilExit
    1. Set isWaitingForExit to true
  3. We are stuck 🤷‍♂️

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, that's not good. Whatever solution we decide to go with, it needs to work with this use case, but without unhandled rejection promises.

Copy link
Author

Choose a reason for hiding this comment

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

Hey @vadimdemedes. I've pushed new implementation, please have a look 🙂

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not a fan of an option that makes waitUntilExit work. As a user of Ink, I'd expect it to just work when I call it.

It looks like this problem isn't solvable without breaking changes. Perhaps we can think what the better API would look like if we weren't constrained by the presence of waitUntilExit and ship this in a major release.

});
2 changes: 1 addition & 1 deletion test/exit.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ test.serial('exit on unmount() with raw mode', async t => {

test.serial('exit with thrown error', async t => {
const output = await run('exit-with-thrown-error');
t.true(output.includes('errored'));
fantua marked this conversation as resolved.
Show resolved Hide resolved
t.true(output.includes('waitUntilExit catch: errored'));
});

test.serial('don’t exit while raw mode is active', async t => {
Expand Down
2 changes: 1 addition & 1 deletion test/fixtures/exit-with-thrown-error.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,5 @@ const app = render(<Test />);
try {
await app.waitUntilExit();
} catch (error: unknown) {
console.log((error as any).message);
console.log(`waitUntilExit catch: ${(error as Error).message}`);
}