Skip to content
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

Web implementation of shared_preferences #2332

Merged
merged 5 commits into from Dec 2, 2019

Conversation

yjbanov
Copy link
Contributor

@yjbanov yjbanov commented Dec 2, 2019

Description

Implement Web support for package:shared_preferences on top of window.localStorage.

Related Issues

Fixes flutter/flutter#35116

@yjbanov yjbanov requested review from ditman and hterkelsen Dec 2, 2019
description: Web platform implementation of shared_preferences
author: Flutter Team <flutter-dev@googlegroups.com>
homepage: https://github.com/flutter/plugins/tree/master/packages/shared_preferences/shared_preferences_web
version: 0.0.1
Copy link
Contributor

@hterkelsen hterkelsen Dec 2, 2019

Choose a reason for hiding this comment

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

Nit: prefer the initial release to be 0.1.0 to get a better score on pub

Copy link
Contributor Author

@yjbanov yjbanov Dec 2, 2019

Choose a reason for hiding this comment

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

Done.

ditman
ditman approved these changes Dec 2, 2019
Copy link
Member

@ditman ditman left a comment

This looks great! I just suggested a few changes, but nothing blocking IMO.

@override
Future<bool> setValue(String valueType, String key, Object value) async {
_checkPrefix(key);
html.window.localStorage[key] = _encodeValue(value);
Copy link
Member

@ditman ditman Dec 2, 2019

Choose a reason for hiding this comment

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

Does it make sense to prepend something to the user-supplied key, to minimize collisions with other things that might be using the shared local storage? shared_preferences or similar?

We can make this transparent on all add/remove operations so users don't even see that we're doing this (unless they go to the dev tools and peek :P)

Copy link
Contributor Author

@yjbanov yjbanov Dec 2, 2019

Choose a reason for hiding this comment

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

The SharedPreferences class from package:shared_preferences already prepends "flutter.". So here I'm just verifying that it's prefixed in _checkPrefix. On top of that, for security reasons, localStorage is scoped to the domain, so you can't step on another site's data.

We could allow customizing the prefix, but currently the value is hard-coded across Dart, Java, and Objective-C code, so it would be a much bigger change.

Copy link
Member

@ditman ditman Dec 2, 2019

Choose a reason for hiding this comment

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

Ah ok, I thought this relied on the user prepending "flutter." to the key themselves. If it's the shared_prefs plugin, I guess it's OK.

I know you can't step on other's site data; the scenario I was envisioning was multiple plugins writing to localStorage on the same site, and overwriting each other's data.


environment:
sdk: ">=2.0.0-dev.28.0 <3.0.0"
flutter: ">=1.5.0 <2.0.0"
Copy link
Member

@ditman ditman Dec 2, 2019

Choose a reason for hiding this comment

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

We haven't been too strict with this so far, but I think that you need a newer version of flutter to support the plugin.platforms syntax of the pubspec.yaml.

I'm not sure what's a good value for the minimal version of flutter yet, though :)

Copy link
Contributor Author

@yjbanov yjbanov Dec 2, 2019

Choose a reason for hiding this comment

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

It's a copy'n'paste from url_launcher. LMK what I should use here instead.

Copy link
Member

@ditman ditman Dec 2, 2019

Choose a reason for hiding this comment

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

I'm not entirely sure, I have a feeling we'll end up coming and updating all of these to the proper version later on, don't worry too much! :P

}

// Check that generics are preserved.
expect((await store.getAll())['flutter.StringList'], isA<List<String>>());
Copy link
Contributor Author

@yjbanov yjbanov Dec 2, 2019

Choose a reason for hiding this comment

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

@ditman this test here shows how the generics are preserved. If you change this to isA<List<int>>(), the test will fail.

Copy link
Member

@ditman ditman Dec 2, 2019

Choose a reason for hiding this comment

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

This is a functionality from the language that I wasn't expecting at all!

dependencies:
...
shared_preferences: ^0.5.4+8
shared_preferences_web: ^0.0.1
Copy link
Contributor

@hterkelsen hterkelsen Dec 2, 2019

Choose a reason for hiding this comment

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

update this line

Copy link
Contributor Author

@yjbanov yjbanov Dec 2, 2019

Choose a reason for hiding this comment

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

Good catch! Done.

@ditman ditman self-requested a review Dec 2, 2019
ditman
ditman approved these changes Dec 2, 2019
Copy link
Member

@ditman ditman left a comment

🚀

@yjbanov yjbanov merged commit 40b4ab0 into flutter:master Dec 2, 2019
42 checks passed
sungmin-park pushed a commit to sungmin-park/flutter-plugins that referenced this issue Dec 17, 2019
Implement `shared_preferences` for the Web.
bykenx pushed a commit to bykenx/flutter-plugins that referenced this issue Dec 12, 2021
Implement `shared_preferences` for the Web.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants