Skip to content

fix(react): Fix Redux integration failing with reducer injection #16106

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Apr 24, 2025

Conversation

AntoineDuComptoirDesPharmacies
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • [ X ] If you've added code that should be tested, please add tests.
  • [ X ] Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Override replaceReducer functiont so any lazy loaded injection will reinsert the sentry reducer in the chain
Add a test to verify that redux enhancer is still calling setContext with updated store avec reducer being replaced
@Lms24 Lms24 requested a review from s1gr1d April 22, 2025 10:41
@Lms24 Lms24 changed the title Bugfix/16017 fix(react): Fix Redux integration failing with reducer injection Apr 22, 2025
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hey @AntoineDuComptoirDesPharmacies thanks for contributing!

I made an adjustment to your PR title for better readability and requested a review from @s1gr1d (as requested in the issue). Please feel free to add any details to your PR description (if necessary). I also started CI for you (needs an approval since you're a first-time contributor).

Will leave the rest up to Sigrid.

Thanks again!

@Lms24 Lms24 linked an issue Apr 22, 2025 that may be closed by this pull request
3 tasks
To keep correct typescript type, use "Proxy" instead of plain object.
@s1gr1d I am wondering if we can do better than accessing the index [0] directly ?
Copy link
Member

@s1gr1d s1gr1d left a comment

Choose a reason for hiding this comment

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

Thanks for adding this PR! I like the clean approach with the Proxy.

Please don't forget to run prettier as the CI complains about it.

// eslint-disable-next-line @typescript-eslint/unbound-method
store.replaceReducer = new Proxy(store.replaceReducer, {
apply: function (target, thisArg, args) {
target.apply(thisArg, [sentryWrapReducer(args[0])]);
Copy link
Member

@s1gr1d s1gr1d Apr 23, 2025

Choose a reason for hiding this comment

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

Just saw your question here: LeComptoirDesPharmacies@631f71e

I am wondering if we can do better than accessing the index [0] directly ?

Not really as we need to use the first argument :/

Run fix:prettier
@s1gr1d s1gr1d merged commit 2e5f6b6 into getsentry:develop Apr 24, 2025
140 checks passed
onurtemizkan pushed a commit that referenced this pull request Apr 24, 2025
)

Before submitting a pull request, please take a look at our

[Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md)
guidelines and verify:

- [ X ] If you've added code that should be tested, please add tests.
- [ X ] Ensure your code lints and the test suite passes (`yarn lint`) &
(`yarn test`).
mydea pushed a commit that referenced this pull request Apr 25, 2025
This PR adds the external contributor to the CHANGELOG.md file, so that
they are credited for their contribution. See #16106

Co-authored-by: s1gr1d <32902192+s1gr1d@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Redux integration failing with reducer injection
3 participants