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

[shared_preferences] Unhardcode Flutter prefix in platform code #2011

Closed
wants to merge 3 commits into from

Conversation

nuzelac
Copy link

@nuzelac nuzelac commented Aug 24, 2019

Description

This is a first step towards resolving the linked issue and being able to access all shared prefs stored in the system. I'm not sure what's the best way to proceed after removing the hardcoded prefix in ios/android. Since prefs is a singleton, maybe adding a new method in Dart that returns all unfiltered prefs, e.g. by calling getAll("")?

Related Issues

flutter/flutter#38769

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

Copy link
Contributor

@collinjackson collinjackson 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 the PR!

I'd prefer to do this PR as a complete and fully tested change that implements desired functionality. You've made some changes to the native-side code, but since prefix is still hard coded on the Dart side, there isn't really an observable difference in behavior for users.

Rather than passing the prefix as an argument to getAll, getString, etc, I think there should be an optional named argument String prefix provided to getInstance (defaulting to '.flutter'). We should maintain a Map of Completer objects on the Dart side corresponding to all the prefixes that have instances created. This will ensure that the Dart-side caches of the SharedPreferences instances will behave properly.

/cc @mehmetf who worked on this plugin recently.

@mehmetf
Copy link
Contributor

mehmetf commented Aug 26, 2019

We cannot accept this PR. This plugin assumes a specific serialization format for certain object types stored in preferences (https://github.com/flutter/plugins/blob/master/packages/shared_preferences/android/src/main/java/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.java#L34-L36). That is why it is namespaced to "flutter."

If you have previously stored preferences in your system that you would like access to, you cannot do so from this plugin reliably. I recommend one of two things:

  • Write a new plugin that is specific to your app (because only you know what type of preferences you stored in there).
  • Write some custom code on the native side to convert the preferences to the format that this plugin accepts.

If #2 sounds reasonable, we should provide an API to register a "migrator". If such a migrator is registered, upon initialization, we can walk through all non-Flutter preferences, call the migrate API and store the returned value in the "flutter." namespace based on its type.

The migrator could look something like this:

enum Type {
  INT,
  FLOAT,
  STRING,
  LIST,
  MAP,
  ...
}

public class FlutterPreferenceValue {
  final Type type;

  // You set one of the following values as determined by [type].
  Integer intValue;
  Float floatValue;
  BigInteger bigIntegerValue;
  String stringValue;
  List listValue;
  Map mapValue;
  ...
}

// You implement this interface and give it to SharedPreferences plugin.
public interface PreferencesMigrator {
  // Only you know how to convert from Object to one of the supported types.
  // If you don't store any List, Map or BigInteger types, this function would be trivial.
  FlutterPreferenceValue migrate(String preferencesKey, Object preferencesValue);
}

@nuzelac
Copy link
Author

nuzelac commented Aug 26, 2019

Thank you for the explanation, I understand why this is not a good approach for the current plugin.

I have only stored strings in the old app so I did not think about the big picture :)
Will do it in a new plugin (and just for the String type) as this is not a common requirement I guess.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants