-
Notifications
You must be signed in to change notification settings - Fork 45
fix(react-native): Enhance fragment detection for indirect references #767
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: main
Are you sure you want to change the base?
fix(react-native): Enhance fragment detection for indirect references #767
Conversation
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.
overall approach seem good to me. Anecdotally traversing imports usually is not that expensive, but it might be nice to do a sanity check benchmark.
Program: { | ||
enter(path, state) { | ||
const fragmentContext = collectFragmentContext(path); | ||
state['sentryFragmentContext'] = fragmentContext; |
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.
We should update AnnotationPluginPass
to strongly type sentryFragmentContext
.
@@ -151,7 +166,8 @@ function functionBodyPushAttributes( | |||
componentName: string, | |||
sourceFileName: string | undefined, | |||
attributeNames: string[], | |||
ignoredComponents: string[] | |||
ignoredComponents: string[], | |||
fragmentContext?: FragmentContext |
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.
Might be time to convert this to take an object instead of plain args (and also add a jsdoc to document)
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.
Sounds reasonable to me -- thanks for adding all the tests!
Besides Abhi's feedback, could you take a look at the integration test? Perhaps we can add a fragment or two there as well (but happy to leave this up to you. If it's too much of a hassle that's fine as well)
Thank you both for the reviews and feedback. I really appreciate this 🙇 |
Fixes getsentry/sentry-react-native#4902