-
Notifications
You must be signed in to change notification settings - Fork 9.7k
[shared_preferences] Merge iOS and macOS implementations #6920
[shared_preferences] Merge iOS and macOS implementations #6920
Conversation
| #if os(iOS) | ||
| import Flutter | ||
| #elseif os(macOS) | ||
| import FlutterMacOS | ||
| #endif |
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.
This would be avoided by flutter/flutter#70413
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.
Yep. This is something I could try to make time for in 2023 maybe?
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 would be really great to unify these frameworks and APIs more, as you know 🙂
| LastUpgradeCheck = 1100; | ||
| ORGANIZATIONNAME = "The Flutter Authors"; | ||
| LastUpgradeCheck = 1300; | ||
| ORGANIZATIONNAME = ""; |
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'm not sure if it's consistent but The Flutter Authors makes sense for example apps. Not important though.
| }, | ||
| { | ||
| "size" : "83.5x83.5", | ||
| "idiom" : "ipad", |
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 know we've talked about this, but do we usually check in the Flutter app icons to example apps in the plugins repo (I can't comment on the binaries themselves)?
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.
Currently we do; I filed flutter/flutter#116614 after earlier discussion but haven't implemented anything yet.
|
At some point were you still hoping to try out an ffi version of shared_preferences? flutter/flutter#111033 |
I'd forgotten about that 😬 But I think it still makes sense to do this for now; the process of consolidating to one implementation package will still be useful in the long term, and other than a couple of ifdefs there's no new code being added. And I'm not sure when we'll do FFI (or even that this plugin will definitely end up working out for it). |
jmagman
left a comment
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.
LGTM
tarrinneal
left a comment
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.
tmtlg
This merges
shared_preferences_iosandshared_preferences_macosinto a newshared_preferences_foundationsthat replaces both of those packages, as described in flutter/flutter#117941:shared_preferences_macostoshared_prefrences_foundation, since the macOS implementation is the Swift implementation, which is what we want to use going forward.darwin/in anticipation of Allow iOS and macOS plugins to share darwin directory flutter#115337), adjusting the code and podspec slightly to make it iOS-compatibleflutter create-ing a new iOS example and wiring it up to use the existing native unit test. (This was done instead of moving the example fromshared_preferences_iossince it seemed better to have the example be in Swift as well now.)shared_preferences_ios.Once this lands and has been published, a follow-up will update
shared_preferencesto use this new package instead of the other two, and the old ones will be marked as deprecated on pub.dev.Part of flutter/flutter#117941
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).