-
-
Notifications
You must be signed in to change notification settings - Fork 610
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
base: master
Are you sure you want to change the base?
Conversation
waitUntilExit()
not resolved with render sync errorswaitUntilExit()
not resolved with sync errors
test/errors.tsx
Outdated
'', | ||
' - Test (test/errors.tsx:22:9)' | ||
] | ||
); | ||
|
||
await t.throwsAsync(waitUntilExit, {message: 'Oh no'}); |
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.
fix for Unhandled rejection in test/errors.tsx
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.
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?
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.
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?
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.
Hm, wasn't that the purpose of this PR? To resolve/reject the promise returned by waitUntilExit
for synchronous errors?
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.
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();
- Execute
render()
- Set
isWaitingForExit
tofalse
- Throw error
- Call
unmout
with error - Don't call
rejectExitPromise
becuaseisWaitingForExit == false
- Set
- Execute
waitUntilExit
- Set
isWaitingForExit
totrue
- Set
- We are stuck 🤷♂️
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.
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.
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.
Hey @vadimdemedes. I've pushed new implementation, please have a look 🙂
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.
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.
Fixes #542