This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Web implementation of shared_preferences #2332
Web implementation of shared_preferences #2332
Changes from 3 commits
0a86ba0
a19063e
f3f175a
79cbc7d
4c37114
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
update this line
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.
Good catch! Done.
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.
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)
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.
The
SharedPreferences
class frompackage: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.
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.
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.
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.
Nit: prefer the initial release to be
0.1.0
to get a better score on pubThere 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.
Done.
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.
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 :)
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's a copy'n'paste from
url_launcher
. LMK what I should use here instead.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 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
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.
@ditman this test here shows how the generics are preserved. If you change this to
isA<List<int>>()
, the test will fail.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 is a functionality from the language that I wasn't expecting at all!