Skip to content
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

feat(react-native): Wrap root app component #835

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

antonis
Copy link
Collaborator

@antonis antonis commented Feb 28, 2025

Fixes #280

Wraps root app component with Sentry.wrap in the following cases:

  1. Simple named exports like export default App;
  2. Named function exports like:
  export default function RootLayout() {
    // function body
  }
  1. Anonymous function exports like:
  export default () => {
    // function body
  }
  1. Simple wrapped exports like export default Another.wrapper(App);

Another approach tested which works for the cases above (and covered by the tests) and may cover even more since it is more generic is the replacing the addSentryWrap method with the following:

function addSentryWrap(js: string): string {
  // Replace export default with export default Sentry.wrap(
  let wrappedJs = js.replace(/export default\s+/g, 'export default Sentry.wrap(');

  // If the file ends with `}`, replace the last `}` with `});`
  if (wrappedJs.trim().endsWith('}')) {
    wrappedJs = wrappedJs.replace(/\}\s*$/, '});');
  }
  // If the file ends with `;`, replace the last `;` with `);`
  else if (wrappedJs.trim().endsWith(';')) {
    wrappedJs = wrappedJs.replace(/;\s*$/, ');');
  }

  return wrappedJs;
}

I chose the more complicated "regex" approach to be more in control of the cases covered, but happy to discuss if this or any other suggestion makes more sense🙏

Success Failure
Screenshot 2025-03-04 at 12 57 08 PM Screenshot 2025-03-04 at 12 56 26 PM

Copy link

github-actions bot commented Feb 28, 2025

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against f30cfee

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 51.25000% with 39 lines in your changes missing coverage. Please review.

Project coverage is 51.22%. Comparing base (2d006c9) to head (f30cfee).

Files with missing lines Patch % Lines
src/react-native/javascript.ts 51.28% 38 Missing ⚠️
src/react-native/react-native-wizard.ts 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   51.15%   51.22%   +0.07%     
==========================================
  Files          53       53              
  Lines        3517     3590      +73     
  Branches      825      831       +6     
==========================================
+ Hits         1799     1839      +40     
- Misses       1591     1750     +159     
+ Partials      127        1     -126     
Flag Coverage Δ
e2e-tests 80.85% <ø> (ø)
unit-tests 50.01% <51.25%> (+0.10%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@antonis antonis requested a review from krystofwoldrich March 4, 2025 16:53
@antonis antonis marked this pull request as ready for review March 4, 2025 16:55
const jsPath = traceStep('find-app-js-file', () =>
getFirstMatchedPath(jsFileGlob),
);
const jsPath = getMainAppFilePath('find-app-js-file');
Copy link
Member

Choose a reason for hiding this comment

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

The traceStep function creates a span, and since those have parent-child relations we can reuse the same name. No need to pass unique for different steps.


const js = fs.readFileSync(jsPath, 'utf-8');

const result = checkAndWrapRootComponent(js);
Copy link
Member

Choose a reason for hiding this comment

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

The Sentry.init used regex mainly because it was written before we started using AST manipulation.

Checkout

export async function patchMetroConfigWithSentrySerializer() {
or/and
export function getOrSetObjectProperty(

Using the AST manipulation will be more safe to adjust the code.

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.

react-native adds wrap during setup
2 participants