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

Refactor the send payload function #203

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

HoussemDjeghri
Copy link
Contributor

@HoussemDjeghri HoussemDjeghri commented Dec 10, 2021

This PR has 2 main changes:

  • Bind the sendBeacon function to the navigator object before passing the reference.
  • Surround the sendBeacon function call with a try catch bloc to catch the url parsing errors.

This PR fixes the issue #194

@ghost
Copy link

ghost commented Dec 10, 2021

CLA assistant check
All CLA requirements met.

@HoussemDjeghri
Copy link
Contributor Author

@sarveshnagpal could you please review this PR, here is an URL example that sendBeacon couldn't parse and throw a "TypeError" or "Illegal Invocation" exception in our prod app earlier today : /email-confirmed?result=%7B%22status%22%3Afalse%7D

packages/clarity-js/src/data/upload.ts Outdated Show resolved Hide resolved
if (dispatched) { done(sequence); }
try {
// sendBeacon must be binded to the navigator before passing the reference
dispatched = navigator.sendBeacon.bind(navigator)(url, payload);
Copy link
Member

Choose a reason for hiding this comment

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

I looked up issue you referenced #194 - do you know if this is a Sentry problem? Looked up MDN: https://developer.mozilla.org/en-US/docs/Web/API/Navigator/sendBeacon and it doesn't require binding to navigator before sending sendBeacon. If you could explain more that will help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarveshnagpal I couldn't find more details also on the manner, but this fix is used to handle the error in popular frameworks like next.js, you can check this commit from the next.js repo , they had the same issue and fixed like I did.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sarveshnagpal here is the issue that mansions this in the next.js repo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sentry also had the same issue and fixed it like that in this PR

@HoussemDjeghri
Copy link
Contributor Author

@sarveshnagpal could you review this PR please, we are still receiving the error in PROD.

@sarveshnagpal sarveshnagpal changed the base branch from master to sarveshn/perf February 2, 2022 14:26
@sarveshnagpal sarveshnagpal merged commit f849e4f into microsoft:sarveshn/perf Feb 2, 2022
sarveshnagpal added a commit that referenced this pull request Feb 2, 2022
* Perf improvement & bug fix to improve Angular SPA

* Fixing lint errors

* Bumping version to 0.6.32

* Bump shelljs from 0.8.4 to 0.8.5 (#209)

Bumps [shelljs](https://github.com/shelljs/shelljs) from 0.8.4 to 0.8.5.
- [Release notes](https://github.com/shelljs/shelljs/releases)
- [Changelog](https://github.com/shelljs/shelljs/blob/master/CHANGELOG.md)
- [Commits](shelljs/shelljs@v0.8.4...v0.8.5)

---
updated-dependencies:
- dependency-name: shelljs
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Refactor the send payload function (#203)

* bind the sendBeacon to navigator before passing reference

* surround the sendBeacon call with try catch bloc

* removed console.error from catch bloc for performance reasons

* Update packages and comments

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Djeghri Houssem <35101902+HoussemDjeghri@users.noreply.github.com>
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

2 participants