feat: reduce jserrors wrapping and remove onerror use #614
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
patrickhousley
added
small
Asset Size ReportMerging this pull request will result in the following CDN asset size changes:
Merging this pull request will result in the following NPM package consumer size changes:
Other Standard CDN AssetsReleased Assets
Built Assets
Other Polyfill CDN AssetsReleased Assets
Built Assets
|
Codecov Report
@@ Coverage Diff @@
## main #614 +/- ##
==========================================
- Coverage 67.01% 66.90% -0.11%
==========================================
Files 129 130 +1
Lines 5990 5965 -25
Branches 1140 1134 -6
==========================================
- Hits 4014 3991 -23
- Misses 1606 1608 +2
+ Partials 370 366 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Tests pass with expected safari 15 errors. |
metal-messiah
approved these changes
Jul 14, 2023
cwli24
reviewed
Jul 14, 2023
| @@ -183,6 +179,7 @@ testDriver.test('string error in custom tracer', function (t, browser, router) { | |||
| var nodeId = interactionTree.children[0].nodeId | |||
|
|
|||
| var error = errors[0] | |||
| console.log(JSON.stringify(error)) | |||
| import { InstrumentBase } from '../../utils/instrument-base' | ||
| import { FEATURE_NAME, NR_ERR_PROP } from '../constants' | ||
| import { FEATURE_NAME } from '../constants' |
There was a problem hiding this comment.
Can you remove NR_ERR_PROP from the constants file too?
Comment on lines
+74
to
+76
| if (typeof error.message !== 'undefined') { | ||
| return new UncaughtError(error.message, error.filename, error.lineno, error.colno) | ||
| } |
There was a problem hiding this comment.
Is there a case when this is called with an error that's not an Error instance yet has a message?
I see fn-err and all internal-error events emitted with an actual error object from try-catch.
There was a problem hiding this comment.
Likewise for castErrorEvent usage
There was a problem hiding this comment.
throw can be passed any value.
| function castReasonToError (reason) { | ||
| let prefix = 'Unhandled Promise Rejection: ' | ||
| if (reason instanceof Error) { | ||
| if (typeof promiseRejectionEvent.reason === 'undefined') return new Error(prefix) |
There was a problem hiding this comment.
Should keep in mind that when we create a new Error, the stack trace is going to point back to this code (us), so users will likely first come to us saying there's some error being thrown in our code.
That said, if they call reject() in promise without an arg, should this be more descriptive than just the prefix string?
There was a problem hiding this comment.
This code was not changed, just moved. That said, I also questioned why we were creating new Error instances here for the same reason. I thought about changing this to use the UncaughtError class.
| globalScope.addEventListener('error', (errorEvent) => { | ||
| if (!this.abortHandler) return | ||
| if (this.#seenErrors.has(errorEvent.error)) { | ||
| this.#seenErrors.delete(errorEvent.error) |
There was a problem hiding this comment.
would you mind adding a comment or some here about the process behind this / why it's done? for others
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replaces the use of the
onerrorglobal function withaddEventListenerto capture errors that go unhandled by customer code. This does not change how unhandled promise rejects are captured. Additional wrappings for events, timers, and animation fames have been removed in favor of capturing unhandled errors using a global event listener.Overview
This PR removes wrappings for events, timers, and animation frames from the jserrors feature. Those wrappings are not specifically necessary unless another feature (spa) needs and performs the wrapping.
Errors are now primarily captured using a global event handler for
errorandunhandledrejectionevents. These events are captured and, if necessary, converted to Error-like objects that can be processed by the aggregator. This removes the use of the globalonerrorfunction and removes the need for customer's to not override ouronerrorfunction.The jserrors feature retains a listener on the event emitter for
fn-errevents. In the event the spa feature is loaded, this handler will capture the error earlier than the global error handlers allowing for the spa feature to properly decorate the error with interaction data.Related Issue(s)
https://issues.newrelic.com/browse/NR-138375
Testing
All the test cases should still pass with this change.