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

Fix waitUntilExit() not resolved with sync errors #563

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

Conversation

fantua
Copy link

@fantua fantua commented Mar 18, 2023

Fixes #542

@fantua fantua changed the title Fix waitUntilExit() not resolved with render sync errors Fix waitUntilExit() not resolved with sync errors Mar 18, 2023
test/errors.tsx Outdated
'',
' - 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

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.

test/exit.tsx Show resolved Hide resolved
@fantua fantua marked this pull request as ready for review March 18, 2023 17:41
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.

waitUntilExit() not resolved with syntax errors
2 participants