Skip to content

Fix: V7 - JavaScript V9 - set undefined for updateProd when not set #4740

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

Open
wants to merge 47 commits into
base: v7
Choose a base branch
from

Conversation

lucas-zimerman
Copy link
Collaborator

@lucas-zimerman lucas-zimerman commented Apr 9, 2025

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

  • Added tests to the wrap function that were missing.
  • Made updateProps always set but with undefined value, avoiding break changes and making the compiler happy.
  • Hide updateProps and children from wrap options.

💡 Motivation and Context

Fixes: #4728

💚 How did you test it?

Unit test, Sample app

📝 Checklist

  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • All tests passing
  • No breaking changes

🔮 Next steps

#skip-changelog.

Copy link
Collaborator

@antonis antonis left a comment

Choose a reason for hiding this comment

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

The code changes and the tests LGMT and aligned with the previous discussion #4568 (comment) 🚀

@krystofwoldrich
Copy link
Member

There is already wrap.test.tsx in the tests. We should add the new tests there.

Since it's TSX, we can use the JSX syntax, which will make the test more readable.

@krystofwoldrich
Copy link
Member

As seen on the above code, we have a tiny margin that this could break (same behavior could happen before this PR) where we set props before triggering shouldComponentUpdate and then set it as undefined and call shouldComponentUpdate, could make it go inside the if condition and break Object.keys since updateProps is undefined.

That could be addressed on a follow up issue since this issue wasn't introduced by this PR.

Let's fix this at once. Looking at the JS implementation, we should pass the appProps to the profiler so it can create spans for the root updates. (js impl)

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

I noticed a few things that should be addressed before merging.

Base automatically changed from lz/bump/jsv9 to v7 April 15, 2025 08:26
…xtra comment / remove unused __esModule parameter. CODE: moved Options definition from wrap function into ReactNativeWrapperOptions / added flag to remove updateProps since we cannot avoid it on ReactNativeProfiler
@lucas-zimerman
Copy link
Collaborator Author

I noticed a few things that should be addressed before merging.

I can't change ReactNativeProfiler to make updateProps optional, since it requires me to make a large rewrite or alter Profiler from @sentry/react to make it optional.

As a way to avoid a big change, I added a flag to remove this parameter when it is included by our wrap function.
Let me know if you have any opinion in regard to it

updateProps: {}
updateProps: options?.profilerProps
Copy link
Member

@krystofwoldrich krystofwoldrich Apr 16, 2025

Choose a reason for hiding this comment

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

I thought we would keep the updateProps: {}? I think just removing the updateProps from the wrap type and here ensuring they will always be {} is enough. I would not delete them in the ReactNativeProfiler class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants