add shared_preferences_platform_interface #2291
Conversation
ditman
left a comment
ditman
left a comment
There was a problem hiding this comment.
This looks pretty good to me!
.-.
: ;
"."
/ \
/ |
.' \
/.' `.\
' \ ``.
_`.____ `-._
/^^^^^^^^`.\^\
/ ` \ (PS)
""""""""""""""""""""""""
SEAL OF APPROVAL
""""""""""""""""""""""""
...preferences/shared_preferences_platform_interface/lib/method_channel_shared_preferences.dart
Outdated
Show resolved
Hide resolved
...erences/shared_preferences_platform_interface/lib/shared_preferences_platform_interface.dart
Outdated
Show resolved
Hide resolved
|
As we discussed offline, please make sure that you add the static |
Done. PTAL. |
harryterkelsen
left a comment
harryterkelsen
left a comment
There was a problem hiding this comment.
You also need to update the homepage: in the pubspec.yaml for the original shared_preferences plugin, since you moved it. And you'll also need to bump the version of the shared_preferences so the new homepage is picked up in pub.
Done: 8466e66 |
|
@hterkelsen I think I found a more ergonomic way for overriding statics, such as |
yjbanov
force-pushed
the
web-shared-preferences
branch
2 times, most recently
from
29f6190 to
100097a
Compare
|
Ok, so after chatting with @hterkelsen we agreed to redesign it. The new design only abstracts away the method channel rather than all of PTAL |
| /// Stores data in memory. | ||
| /// | ||
| /// Data does not persist across application restarts. This is useful in unit-tests. | ||
| class InMemorySharedPreferencesStore extends SharedPreferencesStorePlatform { |
...preferences/shared_preferences_platform_interface/lib/method_channel_shared_preferences.dart
Show resolved
Hide resolved
|
The new version seems to reduce significantly the amount of code that other implementors need to write, I like it! (Note that the analyzer is screaming at a couple of minor things) |
collinjackson
left a comment
collinjackson
left a comment
There was a problem hiding this comment.
I have one suggested change, LGTM otherwise
|
|
||
| /// This does not do anything. | ||
| /// | ||
| /// It is only here for backwards-compatibility. |
There was a problem hiding this comment.
I found this comment confusing. This is the 1.0 version of the platform interface, so there's no need to be backwards compatible. Can we just remove it instead of baking this flaw into the platform API forever? Deprecating/removing commit from the app-facing API can be done later.
There was a problem hiding this comment.
Currently the plugin sends a commit message over to the Java/Objective-C side via a platform channel. I don't know how breaking it would be to stop sending this message, but I was thinking of adding Web support without breaking changes. However, if you are OK with it I'll remove it.
There was a problem hiding this comment.
I just checked the iOS and Android implementations, and they just return true when they get a commit call. I think we should remove it from the platform interface, and when you migrate the plugin, just have commit return true directly.
yjbanov
force-pushed
the
web-shared-preferences
branch
from
765d25e to
fd3518d
Compare
|
Yay, thanks @yjbanov!! |
There was a problem hiding this comment.
@yjbanov this change left analysis_options.yaml in the top level directory shared_preferences instead of also moving it into shared_preferences/shared_preferences, which means that all the existing lint failures from the current shared_preferences plugin will also be ignored in the new shared_platform_interface too. Would it be possible for you to move the lint exclusions to the old package and fix any potentially failing issues in the new one? Also filed flutter/flutter#45627 to track.
* add shared_preferences_platform_interface
6 participants
Description
shared_preferencesone level down to make room for new packages.package:shared_preferences_platform_interfacecontaining the core interface.There are no changes in
package:shared_preferencesyet other than moving the files.Related Issues
First step towards flutter/flutter#35116