-
Couldn't load subscription status.
- Fork 80
fix(ses): fix 2943 error trapping issues #2945
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
base: markm-test-2941-error-traps
Are you sure you want to change the base?
Conversation
37d8080 to
cdca569
Compare
e05786a to
3b18865
Compare
cdca569 to
fbb0ed1
Compare
3b18865 to
4fd18f5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes error trapping issues in SES by improving error reporting when exceptions occur without catch handlers. The fix handles cases where the platform's error event may not contain an actual error object.
- Modified error event handling to fall back to the event object itself when
event.erroris not available - Updated documentation to explain platform-specific behavior differences in error reporting
Reviewed Changes
Copilot reviewed 2 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/ses/src/error/tame-console.js | Adds fallback logic to report the event object when event.error is undefined |
| packages/ses/error-codes/SES_UNCAUGHT_EXCEPTION.md | Documents platform-specific error reporting behavior and fallback mechanism |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
86276e1 to
d3972dd
Compare
4fd18f5 to
079a1e1
Compare
d3972dd to
766a1d8
Compare
079a1e1 to
bcce95e
Compare
766a1d8 to
bdf841f
Compare
bcce95e to
cf6d174
Compare
bdf841f to
7e11152
Compare
cf6d174 to
e28ff91
Compare
|
This is now Ready for Review |
9af8047 to
e5da7f3
Compare
d189d54 to
494e5ce
Compare
Staged on #2944
Closes: #2941
Refs: #XXXX
Description
In browsers, if
errorTrapping:is set to anything other than'none', thenglobalWindow.addEventListener('error', event => {...});is used to log a diagnostic that we wish contain the reason from an unhandled rejection, where the reason should be
event.error.However, in at least some browsers,
event.erroris absent,null, orundefined. We don't care which. In that case, all we've got is the event, so we log that instead.Security Considerations
errorTrappingonly provides a fixed menu, with the repair doing the privileged registration of a closure. This mechanism does not give post-lockdown user code any non-logging abilities it does not otherwise have. And the logging abilities are consistent with the log being write-only.With this PR, an event object does get logged, which may contain info we would have liked to redact for normal user code. But this extra info only appears in the log, from which we never redact info anyway.
Scaling Considerations
none
Documentation Considerations
This PR does add an explanation to SES_UNCAUGHT_EXCEPTION.md, which is where it is most likely to be discovered by those who encounter this.
Testing Considerations
The relevant test were already introduced by #2944 , so this PR could illustrate the fix by the diff between it and #2944.
Compatibility Considerations
none. It is hard to imagine anything was depending on the diagnostic being inadvertently empty.
Upgrade Considerations
none