sugarlabs / sugar-web

Components for Sugar web activities
Apache License 2.0
13 stars 32 forks source link

New module, helper to store localStorage in the datastore #87

Closed manuq closed 10 years ago

manuq commented 11 years ago

v2 of my old try https://github.com/sugarlabs/sugar-web/pull/85

manuq commented 11 years ago

Can be seen at work in GTD activity https://github.com/manuq/gtd-activity

manuq commented 11 years ago

Hey Rogelio,

we initialize dictstore, but the value is asked/stored to localStorage

Yes.

If someone save in localStorage and after that, not use for dictstore.save, the value is stored in localStorage anyways, is this correct?

Yes.

I had something like this in mind:

var storage = Storage.load(callback)
var value = storage.get(key)
storage.save(key, value, callback)

Nice, I can rename init to load.

My first patch https://github.com/sugarlabs/sugar-web/pull/85#commitcomment-4124101 added wrappers for the getter and setter. But dnarvaez and I came to the conclusion that localStorage should be used directly.

I understand that we can not dispense with init/load given the asynchronous nature, right?

Sorry, I don't understand this question.

The save() is needed because there is no way to listen to localStorage changes. There is a "storage" event that is fired when the localStorage change, but in the other windows. See

manuq commented 11 years ago

Another thing: In my first version, the helper module was inside activity/

rogemita commented 11 years ago

What I trying to say is:

Imagine for a moment that the API is as follows, and never we access localstore:

Storage.save (key, value, callback)
var value = Storage.get (key)  !!!

Make that request to get is not possible for asynchronous nature of DatastoreAPI, then one uses init / load in some kind of setup behavior. I was just trying to imagine an API that only has get and save, and below be independent of using localStorage or DatastoreAPI.

manuq commented 11 years ago

2013/11/13 Rogelio notifications@github.com

What I trying to say is:

Imagine for a moment that the API is as follows, and never we access localstore:

Storage.save (key, value, callback) var value = Storage.get (key) !!!

Make that request to get is not possible for asynchronous nature of DatastoreAPI, then one uses init / load in some kind of setup behavior. I was just trying to imagine an API that only has get and save, and below be independent of using localStorage or DatastoreAPI.

We are trying to do it the other way around, using standards as much as possible. Imagine a webapp that uses localStorage, It should be easy to make a web activity of it, adding a minimun of Sugar API.

.. manuq ..

rogemita commented 11 years ago

2013/11/13 manuq notifications@github.com

2013/11/13 Rogelio notifications@github.com

What I trying to say is:

Imagine for a moment that the API is as follows, and never we access localstore:

Storage.save (key, value, callback) var value = Storage.get (key) !!!

Make that request to get is not possible for asynchronous nature of DatastoreAPI, then one uses init / load in some kind of setup behavior. I was just trying to imagine an API that only has get and save, and below be independent of using localStorage or DatastoreAPI.

We are trying to do it the other way around, using standards as much as possible. Imagine a webapp that uses localStorage, It should be easy to make a web activity of it, adding a minimun of Sugar API.

I see now, good point!

.. manuq ..

— Reply to this email directly or view it on GitHubhttps://github.com/sugarlabs/sugar-web/pull/87#issuecomment-28401361 .

Roger.

dnarvaez commented 10 years ago

Bit confused about the status of this pull request... Does it need work? If so maybe close it.

rogemita commented 10 years ago

2013/11/17 Daniel Narvaez notifications@github.com

Bit confused about the status of this pull request... Does it need work? If so maybe close it.

I think no need work, sounds good!, Manu?

init or load I think it is the same, the rest seems fine =)

Roger.

rogemita commented 10 years ago

2013/11/17 Rogelio Mita rogeliomita@gmail.com

I think no need work, sounds good!, Manu?

sorry!!!, forget some "to do items" that were talked by mail, code-sur will deposit them here, and we organize to do them =)

Roger.

code-sur commented 10 years ago

@rogemita, @dnarvaez

there is an issue with the save()... i'm not sure if it's a bug or not (it dependes on how datastoreObject acts when it's not on sugar env):

expect(window.sugar.environment === undefined).toBe.(true);
dictstore.save(callback);

@manuq said that callback would never be called... if it is the case, it needs a bit of work.

I offered myself for adding tests to this module, but I've not been able to find the time yet.

dnarvaez commented 10 years ago

@manuq Should we get your pull request in and then people can improve it in other commits?

@code-sur Are you trying to use the datastore API outside Sugar? I suppose the correct behavior at the moment would be to return an error to the callback. Though maybe it's not worth worrying too much about this, we are going to want a working implementation for it inside a web browser, see the discussions on sugar-devel about Lionel prototype.

code-sur commented 10 years ago

as far as I know, this helper works in order to do some persistence in activities that may work either in standalone mode or under sugar... it tries to use datastore whenever is possible and uses localStorage as a fallback when it isn't.

answering your question, I'm not trying to use datastore outside sugar, on the contrary, I'm saying that this pull will do that whenever you use the "save()" and you aren't on sugar env.

dnarvaez commented 10 years ago

@code-sur I see what you mean now/

dnarvaez commented 10 years ago

I pushed this, let's make improvements in separate commits (fixing the issue @code-sur pointed out for example)

code-sur commented 10 years ago

@dnarvaez, okay... I've made a pull-request with this fix, recently to @manuq 's branch but can do this pull-request to sugarlabs if you agree please, check this out: https://github.com/manuq/sugar-web/pull/2/files

manuq commented 10 years ago

2013/11/18 Code Raguet notifications@github.com

@dnarvaez, okay... I've made a pull-request with this fix, recently to @manuq 's branch but can do this pull-request to sugarlabs if you agree please, check this out: https://github.com/manuq/sugar-web/pull/2/files

Thanks Code. I didn't noticed it. Please do it to master directly.

.. manuq ..

code-sur commented 10 years ago

sure, on my way

code-sur commented 10 years ago

https://github.com/sugarlabs/sugar-web/pull/88