-
Notifications
You must be signed in to change notification settings - Fork 82
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: add support for macOS #217
Conversation
- use symlinks to share code between iOS and macOS, could not use sharedDarwinResource flag
README.md
Outdated
| Dart | Flutter | Amplitude Flutter | Gradle | Android Gradle Plugin | Kotlin Gradle Plugin | | ||
|---------|---------|-------------------|-------|-----------------------|-----------------------| | ||
| `3.7.0+`|`3.3+` | `4.+` | `8.2` | `8.2.2` | `1.9.22` | | ||
|
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.
What requires Dart version? sharedDarwinSource
requires Flutter 3.7 right?
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.
Ah, good catch. Yes, Flutter requires 3.7 for sharedDarwinSource
, but we need dart 3.3+ to support Flutter web after adopting js_interop
. I'll swap those around
darwin/amplitude_flutter.podspec
Outdated
|
||
s.pod_target_xcconfig = { 'DEFINES_MODULE' => 'YES' } | ||
s.swift_version = '5.9' | ||
s.dependency 'AmplitudeSwift', '~> 1.6' |
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.
Can we update to the latest version of the SDK as the min version?
@@ -0,0 +1,14 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> |
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.
Do we need to add this? The underlying SDK also has a privacy manifest which is more up to date.
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.
It was added by default when I tried adding macOS as a platform as a result of me updating to latest flutter locally. I think it should be fine to remove it.
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.
Mostly LGTM with a few minor comments.
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.
Thank you @chungdaniel, LGTM other than flutter & dart version requirements.
- swap dart and flutter versions correctly - use latest version of amplitude swift - remove privacyinfo.xcprivacy
🎉 This PR is included in version 4.0.0-beta.7 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Primary changes:
sharedDarwinSource
to reuse for macOS according to https://docs.flutter.dev/packages-and-plugins/developing-packages#shared-ios-and-macos-implementations - thanks to @jointhejourney for their work in feat: add support for macos #156 for the ideaExtra:
amplitude_flutter.podspec
, follow-up to (iOS)(add-to-app): SWIFT_VERSION missing when compiling the iOS app #211 for v4sharedDarwinSource
Note:
Addresses #46
Jira: [SDK - Flutter] Flutter macOS Support