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

MacOS support #2240

Merged
merged 8 commits into from
May 6, 2022
Merged

MacOS support #2240

merged 8 commits into from
May 6, 2022

Conversation

ospfranco
Copy link
Contributor

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

By conditionally compiling some of the more advanced functionality the SDK now compiles on react-native-macos applications. Don't know how the commented functionality works, XCode was simply throwing errors on the methods, which I guess are not exposed/defined based on platform checks too.

💡 Motivation and Context

I need react-native-macos support for my applications. The commented functionality (performance tracking) is not vital for my use case, for now getting the errors is better than nothing.

💚 How did you test it?

I built my own app with support and the events are being received on Sentry. Not sure how to add tests for the macOS target though, since I didn't change any functionality not sure if this is necessary.

📝 Checklist

  • I reviewed submitted code
  • [] I added tests to verify changes
  • All tests passing
  • No breaking changes

🔮 Next steps

Please review, let me know if you need me to add tests (and if so how/which). If everything is working a prompt release would be much appreciatted

ios/RNSentry.m Outdated Show resolved Hide resolved
ios/RNSentry.m Outdated Show resolved Hide resolved
ios/RNSentry.m Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

The Flutter SDK does the same thing, https://github.com/getsentry/sentry-dart/blob/main/flutter/ios/Classes/SentryFlutterPluginApple.swift
With macCatalyst support as well, can we do that in this PR?
I'd also avoid code duplication.

@ospfranco
Copy link
Contributor Author

Sure, thanks for the review, I'm not very experienced in the native space and also no idea about other sentry features, but the snippets in the FlutterSDK do make a lot of sense :)

@marandaneto
Copy link
Contributor

Sure, thanks for the review, I'm not very experienced in the native space and also no idea about other sentry features, but the snippets in the FlutterSDK do make a lot of sense :)

Happy to guide :) thanks for the PR.

@ospfranco ospfranco requested a review from marandaneto May 4, 2022 18:43
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Thanks for helping us out on this @ospfranco.

ios/RNSentry.m Show resolved Hide resolved
ios/RNSentry.m Outdated Show resolved Hide resolved
ios/RNSentry.m Outdated Show resolved Hide resolved
@marandaneto
Copy link
Contributor

@ospfranco Please add an entry to the CHANGELOG, if CI is happy, I guess we are happy.

Not sure how to add tests for the macOS target though

We have E2E tests running on iOS, so it should be fine as it is.
We don't have a macOS pipeline on CI, but that does not need to be addressed on this PR, rather #1813

Oscar Franco and others added 3 commits May 5, 2022 05:53
Co-authored-by: Manoel Aranda Neto <[email protected]>
Co-authored-by: Manoel Aranda Neto <[email protected]>
@ospfranco
Copy link
Contributor Author

@marandaneto done, someone with access needs to approve the workflow to run the tests

@brustolin
Copy link
Contributor

It looks good to me.

@ospfranco
Copy link
Contributor Author

Tests are not happy, but doesn't look like it is due to my changes.

@ospfranco ospfranco requested a review from marandaneto May 5, 2022 20:02
CHANGELOG.md Outdated Show resolved Hide resolved
@marandaneto marandaneto enabled auto-merge (squash) May 6, 2022 14:21
@marandaneto marandaneto merged commit 41efbfb into getsentry:main May 6, 2022
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.

4 participants