feat(services): implementation local & roaming storage service #298

Odonno · 2016-09-05T21:21:32Z

Here is a basic implementation of local and roaming storage using Windows.Storage api.

What do you think ?

Task list :

Create LocalStorageService
Create RoamingStorageService
Reuse of StorageFileHelper
Sample page

msftclas · 2016-09-05T21:21:35Z

Hi @Odonno, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!
We will now validate the agreement and then real humans will evaluate your PR.

TTYL, MSBOT;

deltakosh · 2016-09-06T18:17:12Z

Microsoft.Toolkit.Uwp.Services/Services/Storage/LocalStorageService.cs

+        /// <typeparam name="T">Type of object retrieved</typeparam>
+        /// <param name="key">Key of the object</param>
+        /// <returns>The T object</returns>
+        public T Retrieve<T>(string key)


what about a default value as well as a third optional parameter?

Would name tit Read to be consistent

That seems possible. 👍

ScottIsAFool · 2016-09-06T18:25:38Z

I'm not 100% convinced these changes fit in the scope of this toolkit. Even if it were, I think there would need to be a large refactor done in order to remove all the duplicated code (since the only thing that changes is the folder/settings location in each implementation. If it's the abstraction that appeals, I would suggest having a look at the Cimbalino toolkit which does this very well with its StorageService and SettingsService.

deltakosh · 2016-09-06T21:35:54Z

Microsoft.Toolkit.Uwp.Services/Services/Storage/ILocalStorageService.cs

+    /// <summary>
+    /// Store data in the Local environment (only on the current device)
+    /// </summary>
+    public interface ILocalStorageService : IStorageService


Why this? I see no value added here

I am commonly using C# interface, mainly with a DI solution. So, in case I want both a local and roaming storage service in my project, I need both interfaces.

Sure but as this is a general framework, we do not want to add more things NGOs to maintain with no obvious reasons. Here an empty interface does not make sense in the context of the toolkit

Ok, I'll remove it

deltakosh · 2016-09-07T19:18:28Z

Moving our conversation back to main thread. I'm for adding this helper to the toolkit but Scott's point is still valid.

deltakosh · 2016-09-07T19:20:13Z

Actually the current version is fine if we remove the saveasync I mentioned. @ScottIsAFool: thoughts?

Odonno · 2016-09-07T20:04:51Z

@deltakosh If we remove the SaveAsync, we should remove the ReadAsync too, no ?

ScottIsAFool · 2016-09-07T20:36:34Z

@Odonno In terms of a toolkit that lets you do this kind of stuff in a line of code, as mentioned previously, Cimbalino allows you to do this, with easy access to either local/roaming storage, etc. I would highly recommend having a look, happy to answer any questions on it if you have any.

Odonno · 2016-09-07T21:03:49Z

@ScottIsAFool I took a look at the documentation but I did not found a strongly typed (generic) implementation of the IStorageServiceHandler, I mean, saving/reading app data with the type (exemple: List<Card> or simply Card).

deltakosh · 2016-09-19T18:54:32Z

Hello, we plan to close v1.1 this monday (9/26). There is no rush but if you want to be part of v1.1 we ahve to merge your code before this Friday

Odonno · 2016-09-19T19:59:40Z

@deltakosh I am sorry, I did not know if it was still maintained. It's good to merge for me.

Could make a sample page if you think it's necessary.

deltakosh · 2016-09-19T20:43:09Z

There is still a debate for sure :) but at least people can try it
I still need more thoughts about whether we want it or not in the toolkit

Odonno · 2016-09-19T20:45:28Z

Ok. I understand.

deltakosh · 2016-09-19T21:04:20Z

@ScottIsAFool pointed out to me that https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/Microsoft.Toolkit.Uwp/Helpers/StorageFileHelper.cs contains already a lot of helpers
You should rely on them

Odonno · 2016-09-20T07:14:05Z

@deltakosh Yeah, that could work.

But I still have two Save/Retrieve methods, one for simple object and another for large object. it's because of the limitation of Settings : The name of each setting can be 255 characters in length at most. Each setting can be up to 8K bytes in size and each composite setting can be up to 64K bytes in size..

deltakosh · 2016-09-20T15:53:17Z

I'm not opposed to keep your class like this but I would like to have it internally use the StorageHelper class

Odonno · 2016-09-25T21:06:00Z

@deltakosh Really good idea. Now, what do you think about a sample page ?

deltakosh · 2016-09-26T17:05:26Z

Definitely! I added helpers section in the sample page.

deltakosh · 2016-09-26T17:06:56Z

I've just noticed that your code is in services and not helpers :(
Can you please move it to toolkit project? (and not toolkit.services)

Odonno · 2016-09-26T17:58:43Z

@deltakosh Ok. I moved the folder to Helpers folder.

deltakosh · 2016-09-26T18:01:32Z

Microsoft.Toolkit.Uwp/project.json

 {
  "dependencies": {
    "Microsoft.NETCore.UniversalWindowsPlatform": "5.1.0",
+    "Newtonsoft.Json": "9.0.1",


You may need to update the nuspec as well:
https://github.com/Microsoft/UWPCommunityToolkit/blob/dev/build/Microsoft.Toolkit.Uwp.nuspec

Odonno · 2016-09-26T18:11:04Z

Ok, it should be good now. I am working on a sample page.

deltakosh · 2016-09-26T18:11:17Z

LGTM

Odonno · 2016-09-26T19:06:36Z

@deltakosh I have started a sample and it looks good.

But there is one bug I don't know how to fix. I create a StorageCode.bind file and when set the property CodeFile, the app crash when I navigate to the page, saying it does not find the file but it shoud... If I remove this property from the json, the navigation works like expected but of course C# code does not appear.

Do you have a solution ?

deltakosh · 2016-09-26T19:10:02Z

It works for all other sample so there must be something wrong in what you did:)

Odonno · 2016-09-26T20:35:43Z

Ok, I found out why it didn't work. I think it's good now.

deltakosh · 2016-09-26T21:31:29Z

build/Microsoft.Toolkit.Uwp.nuspec

    <tags>UWP Toolkit Windows</tags>
+    <dependencies>
+      <group>
+        <dependency id="Newtonsoft.Json" version="[9.0.1,)" />


something wrong with parenthesis right?

What do you mean ? This is a notation to say I only take package with a version >= 9.0.1
If you want the specific version to be 9.0.1 only, I can make the change.

It starts with a [ and ends with a )
Is it expected?

It's like that. I am not in the head of the NuGet team. :)

Ok I understand :) I was not aware of this notation
Thanks a lto!

https://docs.nuget.org/ndocs/create-packages/dependency-versions

deltakosh · 2016-09-26T21:36:50Z

LGTM

deltakosh · 2016-09-27T21:37:18Z

Can you just fix conflicts? I'll merge it :)

deltakosh · 2016-09-27T21:40:52Z

And I may have missed it but we also need a doc here:
https://github.com/Microsoft/UWPCommunityToolkit-docs/tree/dev/en-us/helpers

Odonno · 2016-09-28T19:50:31Z

@deltakosh Shoudl be good now. I'll check the documentation.

msftclas added the cla-signed label Sep 5, 2016

deltakosh reviewed Sep 6, 2016
View reviewed changes

feat(services): implementation local & roaming storage service

f880e28

deltakosh added the cannot merge due to conflicts label Sep 14, 2016

merge

9b7b88b

deltakosh added open discussion ☎️ and removed cannot merge due to conflicts labels Sep 19, 2016

refactor(storage): reuse StorageFileHelper to read/save in files

435e28d

deltakosh reviewed Sep 26, 2016

View reviewed changes

refactor(storage): move service to helpers folder

9b5a8f0

deltakosh approved these changes Sep 26, 2016

View reviewed changes

Merge branch 'dev' into storageService

d340b20

docs(storage): add samples on storage

bedc87c

deltakosh reviewed Sep 26, 2016

View reviewed changes

deltakosh added good to merge I and removed open discussion ☎️ labels Sep 26, 2016

deltakosh added the reviewers wanted label Sep 26, 2016

deltakosh added this to the v1.1 milestone Sep 27, 2016

merge

1bbc56f

deltakosh merged commit 9c3b44c into CommunityToolkit:dev Sep 28, 2016

feat(services): implementation local & roaming storage service #298

feat(services): implementation local & roaming storage service #298

Uh oh!

Conversation

Odonno commented Sep 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msftclas commented Sep 5, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottIsAFool commented Sep 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deltakosh commented Sep 7, 2016

Uh oh!

deltakosh commented Sep 7, 2016

Uh oh!

Odonno commented Sep 7, 2016

Uh oh!

ScottIsAFool commented Sep 7, 2016

Uh oh!

Odonno commented Sep 7, 2016

Uh oh!

deltakosh commented Sep 19, 2016

Uh oh!

Odonno commented Sep 19, 2016

Uh oh!

deltakosh commented Sep 19, 2016

Uh oh!

Odonno commented Sep 19, 2016

Uh oh!

deltakosh commented Sep 19, 2016

Uh oh!

Odonno commented Sep 20, 2016

Uh oh!

deltakosh commented Sep 20, 2016

Uh oh!

Odonno commented Sep 25, 2016

Uh oh!

deltakosh commented Sep 26, 2016

Uh oh!

deltakosh commented Sep 26, 2016

Uh oh!

Odonno commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Odonno commented Sep 26, 2016

Uh oh!

deltakosh commented Sep 26, 2016

Uh oh!

Odonno commented Sep 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deltakosh commented Sep 26, 2016

Uh oh!

Odonno commented Sep 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Odonno commented Sep 5, 2016 •

edited

Loading

Odonno commented Sep 26, 2016 •

edited

Loading