-
Notifications
You must be signed in to change notification settings - Fork 51
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
Changes from 1 commit
3070c36
b44fcc8
034a818
1ee4667
d208ae1
d728ba5
00b9834
4063095
5389cb4
d150d93
016ec87
0fa60c5
039e414
5961ffe
314f7bc
ea68863
4dcfbf5
29e996f
485a65e
5e866c3
f83899e
9c78d93
cb249f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -63,6 +63,24 @@ interface AnnotationPluginPass extends PluginPass { | |||||
|
|
||||||
| type AnnotationPlugin = PluginObj<AnnotationPluginPass>; | ||||||
|
|
||||||
| // Shared context object for all JSX processing functions | ||||||
| interface JSXProcessingContext { | ||||||
| /** Whether to annotate React fragments */ | ||||||
| annotateFragments: boolean; | ||||||
| /** Babel types object */ | ||||||
| t: typeof Babel.types; | ||||||
| /** Name of the React component */ | ||||||
| componentName: string; | ||||||
| /** Source file name (optional) */ | ||||||
| sourceFileName?: string; | ||||||
| /** Array of attribute names [component, element, sourceFile] */ | ||||||
| attributeNames: string[]; | ||||||
| /** Array of component names to ignore */ | ||||||
| ignoredComponents: string[]; | ||||||
| /** Fragment context for identifying React fragments */ | ||||||
| fragmentContext?: FragmentContext; | ||||||
| } | ||||||
|
|
||||||
| // We must export the plugin as default, otherwise the Babel loader will not be able to resolve it when configured using its string identifier | ||||||
| export default function componentNameAnnotatePlugin({ types: t }: typeof Babel): AnnotationPlugin { | ||||||
| return { | ||||||
|
|
@@ -81,16 +99,8 @@ export default function componentNameAnnotatePlugin({ types: t }: typeof Babel): | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| functionBodyPushAttributes( | ||||||
| state.opts["annotate-fragments"] === true, | ||||||
| t, | ||||||
| path, | ||||||
| path.node.id.name, | ||||||
| sourceFileNameFromState(state), | ||||||
| attributeNamesFromState(state), | ||||||
| state.opts.ignoredComponents ?? [], | ||||||
| state.sentryFragmentContext | ||||||
| ); | ||||||
| const context = createJSXProcessingContext(state, t, path.node.id.name); | ||||||
| functionBodyPushAttributes(context, path); | ||||||
| }, | ||||||
| ArrowFunctionExpression(path, state) { | ||||||
| // We're expecting a `VariableDeclarator` like `const MyComponent =` | ||||||
|
|
@@ -110,16 +120,8 @@ export default function componentNameAnnotatePlugin({ types: t }: typeof Babel): | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| functionBodyPushAttributes( | ||||||
| state.opts["annotate-fragments"] === true, | ||||||
| t, | ||||||
| path, | ||||||
| parent.id.name, | ||||||
| sourceFileNameFromState(state), | ||||||
| attributeNamesFromState(state), | ||||||
| state.opts.ignoredComponents ?? [], | ||||||
| state.sentryFragmentContext | ||||||
| ); | ||||||
| const context = createJSXProcessingContext(state, t, parent.id.name); | ||||||
| functionBodyPushAttributes(context, path); | ||||||
| }, | ||||||
| ClassDeclaration(path, state) { | ||||||
| const name = path.get("id"); | ||||||
|
|
@@ -132,7 +134,7 @@ export default function componentNameAnnotatePlugin({ types: t }: typeof Babel): | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| const ignoredComponents = state.opts.ignoredComponents ?? []; | ||||||
| const context = createJSXProcessingContext(state, t, name.node?.name || ""); | ||||||
|
|
||||||
| render.traverse({ | ||||||
| ReturnStatement(returnStatement) { | ||||||
|
|
@@ -142,32 +144,41 @@ export default function componentNameAnnotatePlugin({ types: t }: typeof Babel): | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| processJSX( | ||||||
| state.opts["annotate-fragments"] === true, | ||||||
| t, | ||||||
| arg, | ||||||
| name.node && name.node.name, | ||||||
| sourceFileNameFromState(state), | ||||||
| attributeNamesFromState(state), | ||||||
| ignoredComponents, | ||||||
| state.sentryFragmentContext | ||||||
| ); | ||||||
| processJSX(context, arg); | ||||||
| }, | ||||||
| }); | ||||||
| }, | ||||||
| }, | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| function functionBodyPushAttributes( | ||||||
| annotateFragments: boolean, | ||||||
| /** | ||||||
| * Creates a JSX processing context from the plugin state | ||||||
| */ | ||||||
| function createJSXProcessingContext( | ||||||
| state: AnnotationPluginPass, | ||||||
| t: typeof Babel.types, | ||||||
| path: Babel.NodePath<Babel.types.Function>, | ||||||
| componentName: string, | ||||||
| sourceFileName: string | undefined, | ||||||
| attributeNames: string[], | ||||||
| ignoredComponents: string[], | ||||||
| fragmentContext?: FragmentContext | ||||||
| componentName: string | ||||||
| ): JSXProcessingContext { | ||||||
| return { | ||||||
| annotateFragments: state.opts["annotate-fragments"] === true, | ||||||
| t, | ||||||
| componentName, | ||||||
| sourceFileName: sourceFileNameFromState(state), | ||||||
| attributeNames: attributeNamesFromState(state), | ||||||
| ignoredComponents: state.opts.ignoredComponents ?? [], | ||||||
| fragmentContext: state.sentryFragmentContext, | ||||||
| }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Processes the body of a function to add Sentry tracking attributes to JSX elements. | ||||||
| * Handles various function body structures including direct JSX returns, conditional expressions, | ||||||
| * and nested JSX elements. | ||||||
| */ | ||||||
| function functionBodyPushAttributes( | ||||||
| context: JSXProcessingContext, | ||||||
| path: Babel.NodePath<Babel.types.Function> | ||||||
| ): void { | ||||||
| let jsxNode: Babel.NodePath; | ||||||
|
|
||||||
|
|
@@ -209,29 +220,11 @@ function functionBodyPushAttributes( | |||||
| if (arg.isConditionalExpression()) { | ||||||
| const consequent = arg.get("consequent"); | ||||||
| if (consequent.isJSXFragment() || consequent.isJSXElement()) { | ||||||
| processJSX( | ||||||
| annotateFragments, | ||||||
| t, | ||||||
| consequent, | ||||||
| componentName, | ||||||
| sourceFileName, | ||||||
| attributeNames, | ||||||
| ignoredComponents, | ||||||
| fragmentContext | ||||||
| ); | ||||||
| processJSX(context, consequent); | ||||||
| } | ||||||
| const alternate = arg.get("alternate"); | ||||||
| if (alternate.isJSXFragment() || alternate.isJSXElement()) { | ||||||
| processJSX( | ||||||
| annotateFragments, | ||||||
| t, | ||||||
| alternate, | ||||||
| componentName, | ||||||
| sourceFileName, | ||||||
| attributeNames, | ||||||
| ignoredComponents, | ||||||
| fragmentContext | ||||||
| ); | ||||||
| processJSX(context, alternate); | ||||||
| } | ||||||
| return; | ||||||
| } | ||||||
|
|
@@ -247,45 +240,36 @@ function functionBodyPushAttributes( | |||||
| return; | ||||||
| } | ||||||
|
|
||||||
| processJSX( | ||||||
| annotateFragments, | ||||||
| t, | ||||||
| jsxNode, | ||||||
| componentName, | ||||||
| sourceFileName, | ||||||
| attributeNames, | ||||||
| ignoredComponents, | ||||||
| fragmentContext | ||||||
| ); | ||||||
| processJSX(context, jsxNode); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Recursively processes JSX elements to add Sentry tracking attributes. | ||||||
| * Handles both JSX elements and fragments, applying appropriate attributes | ||||||
| * based on configuration and component context. | ||||||
| */ | ||||||
| function processJSX( | ||||||
| annotateFragments: boolean, | ||||||
| t: typeof Babel.types, | ||||||
| context: JSXProcessingContext, | ||||||
| jsxNode: Babel.NodePath, | ||||||
| componentName: string | null, | ||||||
| sourceFileName: string | undefined, | ||||||
| attributeNames: string[], | ||||||
| ignoredComponents: string[], | ||||||
| fragmentContext?: FragmentContext | ||||||
| componentName?: string | null | ||||||
| ): void { | ||||||
| if (!jsxNode) { | ||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| // Use provided componentName or fall back to context componentName | ||||||
| const currentComponentName = componentName !== undefined ? componentName : context.componentName; | ||||||
|
||||||
| const currentComponentName = componentName !== undefined ? componentName : context.componentName; | |
| const currentComponentName = componentName ?? context.componentName; |
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.
Actually from the code below you do set componentName as null in some function calls, on that context, I believe we should give it a chance for the context.componentName to be set
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.
Good point @lucas-zimerman 👍
Updated with ea68863
Uh oh!
There was an error while loading. Please reload this page.
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.
I feel slightly confused that
componentNamecould be a string,undefinedandnullnow — in which situations it could benullandundefinedand what's the difference between these two cases?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.
Good point @alwx 👍
I tried to simplify this with ea68863