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

Handle failed promises with better messaging #43

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

Conversation

gmarcosb
Copy link

If a promise fails & returns a null value, fail with some additional info

Record args in case promise fails
Throw failure if promise failed
@nknapp
Copy link
Owner

nknapp commented Oct 18, 2024

Could you specify in more detail the input, the helpers and the expected result? That would help me understand what you want to do exactly.

You could also do this by adding a test case to your PR:

  • You probably need a helper that returns a rejecting promise here
  • And a new testcase here

@gmarcosb
Copy link
Author

gmarcosb commented Oct 18, 2024

So this is simply meant to be better logging

Effectively, today if a promise returns a null, the whole thing blows up without clear messaging about what failed ( replacePlaceholdersRecursivelyIn blows up with null inputs here)

This change makes it so that when a promise returns a null value, instead of blowing up in replacePlaceholdersRecursivelyIn with a null access, we blow up before with some information about the promise that returned a null value

In other words, it fails both before & after this change, but after this change it fails with a more helpful message vs no member toHtml on undefined

Additional test: I'm not too famliar with nodejs, mostly just hobbled this together by copy-pasting; there doesn't seem to be any tests for the failure cases, only the happy cases - shouldn't there already be some tests that cover this? 😅 Would happily modify it as that's more straightforward

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.

2 participants