Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Migrate shared_preferences_platform_interfaces to null safety #3466

Merged
bparrishMines merged 4 commits into from
Feb 2, 2021
Merged

Migrate shared_preferences_platform_interfaces to null safety #3466

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d824b03
migrate
bparrishMines Jan 26, 2021
e921808
update nnbd script and remove meta dependency
bparrishMines Jan 26, 2021
0382c6f
null check getAll
bparrishMines Jan 27, 2021
7f696ea
null check instead of cast
bparrishMines Jan 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/shared_preferences/shared_preferences_platform_interface/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.0.0-nullsafety

* Migrate to null safety.

## 1.0.5

* Update Flutter SDK constraint.
Expand Down
40 changes: 15 additions & 25 deletions ...ferences/shared_preferences_platform_interface/lib/method_channel_shared_preferences.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,39 +18,29 @@ const MethodChannel _kChannel =
class MethodChannelSharedPreferencesStore
extends SharedPreferencesStorePlatform {
@override
Future<bool> remove(String key) {
return _invokeBoolMethod('remove', <String, dynamic>{
'key': key,
});
Future<bool> remove(String key) async {
return await _kChannel.invokeMethod<bool>(
'remove',
<String, dynamic>{'key': key},
) as bool;
}

@override
Future<bool> setValue(String valueType, String key, Object value) {
return _invokeBoolMethod('set$valueType', <String, dynamic>{
'key': key,
'value': value,
});
}

Future<bool> _invokeBoolMethod(String method, Map<String, dynamic> params) {
return _kChannel
.invokeMethod<bool>(method, params)
// TODO(yjbanov): I copied this from the original
// shared_preferences.dart implementation, but I
// actually do not know why it's necessary to pipe the
// result through an identity function.
//
// Source: https://github.com/flutter/plugins/blob/3a87296a40a2624d200917d58f036baa9fb18df8/packages/shared_preferences/lib/shared_preferences.dart#L134
.then<bool>((dynamic result) => result);
Future<bool> setValue(String valueType, String key, Object value) async {
return await _kChannel.invokeMethod<bool>(
'set$valueType',
<String, dynamic>{'key': key, 'value': value},
) as bool;
}

@override
Future<bool> clear() {
return _kChannel.invokeMethod<bool>('clear');
Future<bool> clear() async {
return await _kChannel.invokeMethod<bool>('clear') as bool;
}

@override
Future<Map<String, Object>> getAll() {
return _kChannel.invokeMapMethod<String, Object>('getAll');
Future<Map<String, Object>> getAll() async {

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Are we certain that no platform implementation ever returns null here? It might be a good idea to bullet-proof this with a null check on the results, returning an empty map if the channel's result was null.

@bparrishMines bparrishMines Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

return await _kChannel.invokeMapMethod<String, Object>('getAll')
as Map<String, Object>;

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we need the cast? I thought the purpose of invokeMapMethod was do to that cast internally.

@bparrishMines bparrishMines Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

All the invoke methods return the nullable version of the type parameters. As in invokeMapMethod<String, Object> returns Map<String, Object>?.

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wouldn't that just need a ! though, rather than a cast?

@bparrishMines bparrishMines Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes that would work but you would have to add parentheses around the await keyword. Basically the two options are

final Map<String, Object> map = (await invokeMapMethod<String, Object>())!;

or

final Map<String, Object> map = await invokeMapMethod<String, Object>() as Map<String, Object>;

I don't really have a preference on which is better. I think the top one is shorter, but the second is easier to read/cleaner.

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd vote strongly for the top one, for the same reason that people prefer C++-style casts to C-style casts: it's explicit about what it's doing. ! does exactly one thing (remove the nullability from the type), while as could do other kinds of casting. The fact that I didn't know what the cast was for in the second form seems like pretty good evidence that it has readability issues.

Plus, if there were a mistake with the types here (say, a mismatch in the contents of the <> in invokeMapMethod vs the cast), the as would be a runtime failure, while the ! would be a compile-time failure.

}
}
2 changes: 1 addition & 1 deletion ...nces/shared_preferences_platform_interface/lib/shared_preferences_platform_interface.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import 'dart:async';

import 'package:meta/meta.dart';
import 'package:flutter/foundation.dart';

import 'method_channel_shared_preferences.dart';

Expand Down
7 changes: 3 additions & 4 deletions packages/shared_preferences/shared_preferences_platform_interface/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
name: shared_preferences_platform_interface
description: A common platform interface for the shared_preferences plugin.
homepage: https://github.com/flutter/plugins/tree/master/packages/shared_preferences/shared_preferences_platform_interface
version: 1.0.5
version: 2.0.0-nullsafety

dependencies:
meta: ^1.0.4
flutter:
sdk: flutter

dev_dependencies:
flutter_test:
sdk: flutter
pedantic: ^1.8.0
pedantic: ^1.10.0-nullsafety

environment:
sdk: ">=2.1.0 <3.0.0"
sdk: ">=2.12.0-0 <3.0.0"
flutter: ">=1.12.8"
14 changes: 6 additions & 8 deletions ...es/shared_preferences_platform_interface/test/method_channel_shared_preferences_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,18 @@ void main() {
'plugins.flutter.io/shared_preferences',
);

const Map<String, dynamic> kTestValues = <String, dynamic>{
const Map<String, Object> kTestValues = <String, Object>{
'flutter.String': 'hello world',
'flutter.Bool': true,
'flutter.Int': 42,
'flutter.Double': 3.14159,
'flutter.StringList': <String>['foo', 'bar'],
};

InMemorySharedPreferencesStore testData;
late InMemorySharedPreferencesStore testData;

final List<MethodCall> log = <MethodCall>[];
MethodChannelSharedPreferencesStore store;
late MethodChannelSharedPreferencesStore store;

setUp(() async {
testData = InMemorySharedPreferencesStore.empty();
Expand All @@ -44,9 +44,9 @@ void main() {
return await testData.clear();
}
final RegExp setterRegExp = RegExp(r'set(.*)');
final Match match = setterRegExp.matchAsPrefix(methodCall.method);
if (match.groupCount == 1) {
final String valueType = match.group(1);
final Match? match = setterRegExp.matchAsPrefix(methodCall.method);
if (match != null && match.groupCount == 1) {

@stuartmorgan-g stuartmorgan-g Jan 27, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: couldn't this be if (match?.groupCount == 1) (since null != 1)?

@bparrishMines bparrishMines Jan 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah, good suggestion. done

final String valueType = match.group(1)!;
final String key = methodCall.arguments['key'];
final Object value = methodCall.arguments['value'];
return await testData.setValue(valueType, key, value);
Expand All @@ -59,8 +59,6 @@ void main() {

tearDown(() async {
await testData.clear();
store = null;
testData = null;
});

test('getAll', () async {
Expand Down
2 changes: 1 addition & 1 deletion script/nnbd_plugins.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ readonly NNBD_PLUGINS_LIST=(
"path_provider"
"plugin_platform_interface"
"share"
"shared_preferences"
"url_launcher"
"video_player"
"webview_flutter"
Expand All @@ -34,7 +35,6 @@ readonly NON_NNBD_PLUGINS_LIST=(
# "in_app_purchase"
# "quick_actions"
# "sensors"
# "shared_preferences"
# "wifi_info_flutter"
)

Expand Down