Uh oh!
There was an error while loading. Please reload this page.
feat(redux): Add 'attachReduxState' option - #8953
Conversation
Introduces 'attachReduxState' (default: true) for controlling Redux state attachment to Sentry events. FixesgetsentryGH-6266
HazAT
left a comment
There was a problem hiding this comment.
This is awesome, looking forward merging this!
Maybe we can add a small test for this, should be too hard since we already have quite a few tests for Redux.
Uh oh!
There was an error while loading. Please reload this page.
malay44
commented
Sep 6, 2023
Thanks for the positive feedback! I'll add a test for this feature and let you know the progress. 🤗
|
Uh oh!
There was an error while loading. Please reload this page.
| const ACTION_BREADCRUMB_TYPE = 'info'; | ||
| const defaultOptions: SentryEnhancerOptions = { | ||
| attachReduxState: true, |
There was a problem hiding this comment.
I'm not sure we can default this to be true because attachments have their own quota. Thoughts @HazAT?
There was a problem hiding this comment.
hmm I think it's fine - Redux while popular is not in every React app - and this feature is very helpful.
There was a problem hiding this comment.
Alright let's
after we get some tests in @malay44!
35fa934 to
2b3735aCompareRefactors the code responsible for attaching the Redux state to Sentry events. The change enhances efficiency by avoiding the addition of empty JSON attachments when there is no Redux state available.
| event.contexts && | ||
| event.contexts.state && | ||
| event.contexts.state.state && | ||
| event.contexts.state.state.type === 'redux' |
There was a problem hiding this comment.
What if we add an extra condition, such as event instanceof ErrorEvent ? I noticed that when I was debugging in the browser, it was also attaching the Redux state when the event type was 'transaction'
There was a problem hiding this comment.
yup we should do this.
to filter for just errors you can add event.type === undefined. The reason there is no error type is because of backwards compatibility 😢 (error is undefined, all other events have a type).
Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:
try{// @ts-expect-error try catch to reduce bundle sizeif(event.type===undefined&&event.contexts.state.state.type==='redux'){hint.attachments=[
...(hint.attachments||[]),// @ts-expect-error try catch to reduce bundle size{filename: 'redux_state.json',data: JSON.stringify(event.contexts.state.state.value)},];}}catch(_){// empty}There was a problem hiding this comment.
Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:
This article is fantastic! I've learned a lot while working on this issue. Thank you all so much for your guidance; I appreciate it.
I'm eager to take on more challenging issues. Please consider assigning some to me. I'm looking forward to learning and growing while contributing to the project.
New test cases to validate the attachment of Redux state to Sentry events. The test ensures that the attachment logic correctly adds the Redux state when available and does not add it when it's empty or undefined and when not of type redux.
AbhiPrasad
left a comment
There was a problem hiding this comment.
thanks for adding tests!
Uh oh!
There was an error while loading. Please reload this page.
| Sentry.configureScope(scope => { | ||
| expect(scope.getAttachments()).toContainEqual( | ||
| expect.objectContaining({ | ||
| filename: 'redux_state.json', | ||
| data: JSON.stringify({ | ||
| value: 'updated', | ||
| }), | ||
| }), | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
We can access the attachments on the scope via Sentry.getCurrentHub().getScope().getAttachments() - let's do that for the rest of the uses of configureScope as well.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
I discovered today that I made an error when writing the tests. They currently pass for every scenario. I'll investigate this issue further and let you know how I'm doing.
dff0eb2 to
4a7e091CompareRefined the test suite for Redux state attachment by: 1. Removing an incorrect test from`reduxStateAttachments.test.ts`. 2. Adding a new test case to `redux.ts` to verify that the attachment logic correctly attaches the Redux state when available, avoids it when it's empty or undefined, and verifies that it's of the correct type 'redux'.
AbhiPrasad
left a comment
There was a problem hiding this comment.
let's make the redux state only attach for errors and add a test for this, and we are good to merge.
| event.contexts && | ||
| event.contexts.state && | ||
| event.contexts.state.state && | ||
| event.contexts.state.state.type === 'redux' |
There was a problem hiding this comment.
yup we should do this.
to filter for just errors you can add event.type === undefined. The reason there is no error type is because of backwards compatibility 😢 (error is undefined, all other events have a type).
Also, if you put this whole block into a try catch you can skip this deep nested check, which helps save bundle size:
try{// @ts-expect-error try catch to reduce bundle sizeif(event.type===undefined&&event.contexts.state.state.type==='redux'){hint.attachments=[
...(hint.attachments||[]),// @ts-expect-error try catch to reduce bundle size{filename: 'redux_state.json',data: JSON.stringify(event.contexts.state.state.value)},];}}catch(_){// empty}Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Abhijeet Prasad <devabhiprasad@gmail.com>
This pull request introduces a new configuration option,
attachReduxState, with a default value oftrue. This option lets users control whether the Redux state should automatically attach to Sentry events.Changes Made:
attachReduxStateconfiguration option, default true.attachReduxStateis set totrue.redux.Motivation:
By default, the 'attachReduxState' option is set to true, enabling the attachment of the Redux state to Sentry events. This enhancement improves error tracking for Redux-based applications.
Testing:
I have thoroughly tested these changes and confirmed that they work as intended.
Related Issues:
ClosesGH-6266
Screenshots:
