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

loadScript rejects with an Error #391

Merged
merged 4 commits into from
Feb 1, 2023
Merged

Conversation

arelra
Copy link
Member

@arelra arelra commented Feb 1, 2023

Background

The current most frequent error in Sentry, accounting for ~20% of DCR's errors is:

Non-Error exception captured with keys: currentTarget, isTrusted, target, type

This is a generic error from Sentry telling us that it requires rejected promises to return a proper Error:

getsentry/sentry-javascript#2546 (comment)

https://sentry.zendesk.com/hc/en-us/articles/360057389753-Why-am-I-seeing-events-with-Non-Error-exception-or-promise-rejection-captured-with-keys-using-the-JavaScript-SDK-

The errors look to be coming from loadScript given the serialised object reported by Sentry e.g.:

{
  currentTarget: [object Null], 
  isTrusted: True, 
  target: head > script#ipsos[type="text/javascript"], 
  type: error
}
{
  currentTarget: [object Null], 
  isTrusted: True, 
  target: head > script, 
  type: error
}

This change

Updates promise rejections to return an Error type.

Changing the promise rejection won't reduce the errors but it will give us better insight into the errors reported by Sentry.

I suspect the underlying errors are the result of ad-blockers but we can't be sure without more detailed errors.

Why?

To get better insight into high volume errors reported by Sentry

@arelra arelra requested a review from a team as a code owner February 1, 2023 13:25
@changeset-bot
Copy link

changeset-bot bot commented Feb 1, 2023

🦋 Changeset detected

Latest commit: 9ebd68b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@guardian/libs Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@arelra arelra requested a review from a team February 1, 2023 13:25
@github-actions github-actions bot added @guardian/libs 📦 npm Affects a @guardian package on NPM labels Feb 1, 2023
@arelra arelra force-pushed the ravi/loadscript-rejects-with-error branch from 68ed5a0 to 015b9f1 Compare February 1, 2023 14:07
@arelra arelra force-pushed the ravi/loadscript-rejects-with-error branch from 015b9f1 to b4104c1 Compare February 1, 2023 15:00
@arelra
Copy link
Member Author

arelra commented Feb 1, 2023

@ob6160 @joecowton1

fyi the checks were stuck in pending status as a conversation was not resolved.

Once I resolved the conversation however the checks stayed stuck in a pending state 😢

I've done a no-edit commit and force pushed to kick it off again.

@joecowton1
Copy link
Contributor

@ob6160 @joecowton1

fyi the checks were stuck in pending status as a conversation was not resolved.

Once I resolved the conversation however the checks stayed stuck in a pending state 😢

I've done a no-edit commit and force pushed to kick it off again.

Look ok now, let us know if its not!

@arelra arelra requested a review from sndrs February 1, 2023 17:01
@arelra arelra merged commit 64c2e7f into main Feb 1, 2023
@arelra arelra deleted the ravi/loadscript-rejects-with-error branch February 1, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@guardian/libs 📦 npm Affects a @guardian package on NPM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants