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

feat: Add error callback #413

Merged
merged 6 commits into from
Jul 30, 2021
Merged

feat: Add error callback #413

merged 6 commits into from
Jul 30, 2021

Conversation

ajhorst
Copy link
Contributor

@ajhorst ajhorst commented Jul 30, 2021

Summary

Adds an error callback to the various event logging methods

The tricky part to this is that the existing callback has very specific behavior not to be called when there's an error, except in the case where the error is with the state of the sdk client. In order to not break that behavior, I did the following

  • call both the existing callback and the new error callback wherever just the old callback was previously called for errors (not instantiating the API key, etc)
  • Calling just the error callback when a log request returns with something besides a 200

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

https://amplitude.atlassian.net/browse/AMP-35348

Copy link
Contributor

@kelsonpw kelsonpw left a comment

Choose a reason for hiding this comment

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

Nice work!

@ajhorst ajhorst changed the title Add error callback feat: Add error callback Jul 30, 2021
@ajhorst
Copy link
Contributor Author

ajhorst commented Jul 30, 2021

Found a JIRA ticket from the last time this was encountered https://amplitude.atlassian.net/browse/AMP-35348

Copy link
Contributor

@jooohhn jooohhn left a comment

Choose a reason for hiding this comment

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

LGTM, 1 small nit

@@ -1468,6 +1525,28 @@ if (BUILD_COMPAT_2_0) {
};
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: since logErrorsOnEvents is private, should it be _logErrorsOnEvents for convention?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point, lemme fix that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jooohhn done

@ajhorst ajhorst merged commit c50429d into main Jul 30, 2021
@ajhorst ajhorst deleted the add_error_callback branch July 30, 2021 21:26
github-actions bot pushed a commit that referenced this pull request Jul 30, 2021
# [8.4.0](v8.3.1...v8.4.0) (2021-07-30)

### Features

* Add error callback ([#413](#413)) ([c50429d](c50429d))
@gigiRa
Copy link

gigiRa commented Jan 26, 2022

Still having the same issue

Hey there!

Today I installed amplitude-js 8.16.0, and it's not working when uBlock is turned on.

When I switch it off, it works just fine.

Any idea how to solve this?

Thanks!

@ajhorst
Copy link
Contributor Author

ajhorst commented Jan 26, 2022

Hi @gigiRa, this fix allows developers to detect when the JS SDK is blocked by uBlock or other blocker extensions, but it cannot bypass the extensions by itself.

If you're looking for a way to work around a blocking extension, one solution Amplitude recommends is to set up a proxy on your own servers https://developers.amplitude.com/docs/domain-proxies

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.

None yet

4 participants