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

Allow passing an error on exit #165

Merged

Conversation

mAAdhaTTah
Copy link
Contributor

Passing a result to exit will cause waitUntilExit to reject
with the value as the error.

Fixes #163.

readme.md Outdated Show resolved Hide resolved
src/instance.js Show resolved Hide resolved
@sindresorhus
Copy link
Collaborator

sindresorhus commented Mar 18, 2019

Why do you need to pass the error to exit() to have waitUntilExit() reject? I would have expected throwing an error anywhere in the Ink app would make waitUntilExit() reject.

@sindresorhus
Copy link
Collaborator

You need to update https://github.com/vadimdemedes/ink/blob/master/index.d.ts

@mAAdhaTTah mAAdhaTTah force-pushed the pass-error-object-on-exit branch 3 times, most recently from f58669f to 07ff24b Compare March 18, 2019 12:10
@mAAdhaTTah
Copy link
Contributor Author

Why do you need to pass the error to exit() to have waitUntilExit() reject? I would have expected throwing an error anywhere in the Ink app would make waitUntilExit() reject.

That seems like a larger issue to solve than this. I'm not sure how to add the plumbing to handle that unless React does a bunch of that work for us.

@vadimdemedes
Copy link
Owner

Why do you need to pass the error to exit() to have waitUntilExit() reject? I would have expected throwing an error anywhere in the Ink app would make waitUntilExit() reject.

That's actually a good idea. We already have an <App> component which wraps the whole Ink app. We could add componentDidCatch() there, which would reject waitUntilExit(). Feel free to leave it out of this PR, we can tackle it separately.

@mAAdhaTTah
Copy link
Contributor Author

We already have an <App> component which wraps the whole Ink app. We could add componentDidCatch() there, which would reject waitUntilExit().

I dig it. I'd be willing to address that in a separate PR.

@mAAdhaTTah
Copy link
Contributor Author

Updated.

@sindresorhus
Copy link
Collaborator

@mAAdhaTTah
Copy link
Contributor Author

@sindresorhus I made that change. The #unmount on the App needs to take the error, but I updated the returned instance from render so it doesn't get passed along.

@mAAdhaTTah
Copy link
Contributor Author

Minor adjustment made: I updated the test case to use the error object for console.log, so we verify that the error object is actually passed through the rejected promise.

@vadimdemedes
Copy link
Owner

Thanks @mAAdhaTTah, great work!

@vadimdemedes
Copy link
Owner

Could you fix the merge conflict, so that we can get it to master? ;)

Passing a result to `exit` will cause `waitUntilExit` to reject
with the value as the error.

Fixes vadimdemedes#163.
@mAAdhaTTah
Copy link
Contributor Author

Resolved.

@vadimdemedes
Copy link
Owner

Thanks @mAAdhaTTah, I will take another look tomorrow and merge it!

@vadimdemedes vadimdemedes merged commit dcaaafa into vadimdemedes:master Mar 27, 2019
@mAAdhaTTah mAAdhaTTah deleted the pass-error-object-on-exit branch March 29, 2019 11:39
@mAAdhaTTah
Copy link
Contributor Author

@vadimdemedes Not to be a pain, but a release with this fix + #170 would be really helpful for the CLI I'm working on. Thanks!

@vadimdemedes
Copy link
Owner

@mAAdhaTTah Sorry for the delay, I pushed some big changes in the core and wanted to give people a few a days to test them out. Going to release in a few minutes ;)

@vadimdemedes
Copy link
Owner

New release is out - https://github.com/vadimdemedes/ink/releases/tag/v2.1.0 🎆

@mAAdhaTTah
Copy link
Contributor Author

No worries, thank you!

jedahan pushed a commit to jedahan/ink-1 that referenced this pull request Sep 7, 2019
* Allow passing an error on exit

Passing a result to `exit` will cause `waitUntilExit` to reject
with the value as the error.

Fixes vadimdemedes#163.

* Update readme.md
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.

3 participants