Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add error callback #27

Merged

Conversation

wilson-ascenda
Copy link
Contributor

Summary

Make ngx-turnstile emit error code when the widget encounters an error.

Motivation

Currently, ngx-turnstile doesn't pass any error-callback configuration to the Turnstile widget, so by default "Turnstile will throw a JavaScript exception upon error". This may cause quite a lot of noise in error tracking systems like Rollbar.

This PR adds an error-callback that emits the error code as a component output.

ngx-turnstile users can subscribe to this error code and handle it according to their needs, which helps to address issue #26.

Caveats

Ideally, the ngx-turnstile component should accept a callback Function as input:

  • This function should take a string and return a boolean.
  • If no such function was provided, ngx-turnstile should not pass the 'error-callback' option to Turnstile.

However, I'm not sure how to implement that, so I've replicated the EventEmitter approach from the resolved output. Now, in order to intercept the error code, ngx-turnstile will always pass 'error-callback' to the Turnstile widget.

Warning: breaking changes

The caveats above would mean that ngx-turnstile users will no longer see exceptions being thrown. Instead, they'll see the error code logged as a console warning.

(It would be great if someone could help with avoiding this breaking change!)

Copy link
Member

@choyiny choyiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for your contribution! lgtm

@wilson-ascenda
Copy link
Contributor Author

Hi @choyiny @AleksanderBodurri

Thank you for the reviews, can we proceed to merge & release the changes please?

@AleksanderBodurri
Copy link
Contributor

Thanks for the PR @wilson-ascenda 🙏 Will publish the latest release later today

@AleksanderBodurri AleksanderBodurri merged commit 3deb99b into verto-health:main Apr 24, 2024
1 check passed
@wilson-ascenda
Copy link
Contributor Author

Great to hear. Thanks!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants