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

Avoid leaking unhandled rejections #394

Merged
merged 1 commit into from Sep 10, 2023
Merged

Conversation

shicks
Copy link
Contributor

@shicks shicks commented Sep 1, 2023

Fixes #393.

Previously the callback-based API leaked unhandled rejections in two places:

  1. After calling the callback, it rejected the unreturned promise, which is never handled. This is fixed by returning between the callback and the reject.
  2. While watching, the first build unexpectedly returns a promise reflecting the initial result. If it's failed, it's likely to go unhandled, but also doesn't ever call the callback anyway. This is fixed by passing the callback for failures, such that the returned promise is the (probably-successful) result of the callback, rather than the error itself.

Fixes metalsmith#393.  Previously the callback-based API leaked unhandled rejections in two places:
1. After calling the callback, it rejected the unreturned promise, which is never handled. This is fixed by returning between the callback and the reject.
2. While watching, the first build unexpectedly returns a promise reflecting the initial result. If it's failed, it's likely to go unhandled, but also doesn't ever call the callback anyway. This is fixed by passing the callback for failures, such that the returned promise is the (probably-successful) result of the callback, rather than the error itself.
@webketje
Copy link
Member

webketje commented Sep 10, 2023

I've reviewed these changes locally. I haven't added unit tests yet, but I feel this is already sufficiently valuable to include in a patch release, thank you!

@webketje webketje merged commit 878fb79 into metalsmith:master Sep 10, 2023
5 checks passed
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.

Unhandled rejection when using callbacks (i.e. with .watch(spec))
2 participants