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

Add sentry serializer to RN metro config #502

Merged
merged 11 commits into from
Nov 21, 2023
Merged

Conversation

krystofwoldrich
Copy link
Member

@krystofwoldrich krystofwoldrich commented Nov 20, 2023

This PR adds ability to patch metro.config.js in RN project and automatically add the Sentry Metro plugin as released in https://github.com/getsentry/sentry-react-native/releases/tag/5.11.0

Example patch:

const {createSentryMetroSerializer} = require('@sentry/react-native/dist/js/tools/sentryMetroSerializer');
const config = {serializer: {customSerializer: createSentryMetroSerializer()}};

@krystofwoldrich krystofwoldrich self-assigned this Nov 20, 2023
@krystofwoldrich krystofwoldrich marked this pull request as ready for review November 20, 2023 18:41
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.

LGTM from general wizard perspective! Also thanks for adding the tests - makes it much easier to understand what's going on when reviewing

src/react-native/metro.ts Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

General comment/not relevant for this PR: AFAICT, the RN wizard is the only one that supports uninstall logic (maybe electron has it as well, not sure). I was wondering if we should consider dropping uninstall support since most of our wizard flows don't do this and it takes a considerable amount logic to work properly. WDYT?

Btw, not saying we have to do this, just a thought. I'm happy to keep it around but at the same time I think it's unlikely that we'll add it to JS or source maps wizards.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, it adds a bit of overhead. We could drop it in the future in favor of a doc page documenting the uninstall steps. Or we can limit it to the native parts which are in general had for JS dev to unpatch.

@krystofwoldrich krystofwoldrich merged commit ef04c29 into master Nov 21, 2023
@krystofwoldrich krystofwoldrich deleted the kw-rn-metro-plugin branch November 21, 2023 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants