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

Feature: Storage API #435

Open
akinsho opened this issue Apr 5, 2019 · 10 comments
Open

Feature: Storage API #435

akinsho opened this issue Apr 5, 2019 · 10 comments
Labels
A-other Area: Other, does not fit neatly into any of the other buckets enhancement New feature or request

Comments

@akinsho
Copy link
Member

akinsho commented Apr 5, 2019

I think a storage API similar to local storage, with a similar (within reason) api. So that apps could use revery to persist information across sessions.

We could store the app's data somwhere similar to where applications like chrome store localStorage in chrome it is %LocalAppData%\Google\Chrome\User Data\Default\Local Storage on windows. We would ideally do this somwhere like this or the equivalent on mac and linux (N.B. not in the same place).

We would need to implement the api like

setItem(key: string, value: string): unit
getItem(key: string): string
removeItem(key: string): unit

and write/append that to a json file in the location(s) above.

@tatchi
Copy link
Contributor

tatchi commented Apr 9, 2019

Hi,

Would love to help on that if it's ok for you. Note that I'm still new in this OCaml/ReasonML world and would for sure need some helps/guidance.

@Et7f3
Copy link
Member

Et7f3 commented Apr 9, 2019

why in chrome folder ? @Akin909
why don't have one file per value ? instead of mix all in one json file ?

@akinsho
Copy link
Member Author

akinsho commented Apr 9, 2019

@Et7f3 sorry I wrote that in a rush definitely not in the same place, meant to just use that as an example of how google chrome do it so somewhere in local app data on windows and wherever the equivalent is on mac and linux

@Et7f3
Copy link
Member

Et7f3 commented Apr 9, 2019

Ok.
the standard folder on Mac OS is $HOME/Library
as we can see in tis example
/Users/pseudo/Library/Application Support/Google/Chrome/Default

Don't know where is this file on linux but we can add $HOME/.config or $HOME/.revery/name_of_executable

what about one value per file to have direct access ? (it will be then easy to add this feature)

@bryphe
Copy link
Member

bryphe commented Apr 12, 2019

@tatchi - thanks for offering to help out! This would be really nice convenience API to have (I could see using it for Onivim 2 for persisting user settings / state, like window size, for example).

Regarding the directories - @jordwalke is actually working on a library that would be very helpful for us: https://github.com/facebookexperimental/reason-native/blob/83c4b0ec170664767cb75dd2673f1533e6036a47/src/dir/README.md

API is still TBD but we could vendor it here to test it out and give feedback... would make it really easy to get the appropriate directory on each platform (ie, Dir.dataLocal)

@msvbg
Copy link
Contributor

msvbg commented Apr 12, 2019

It may be worth asking what belongs in revery core and what belongs in userland. This seems like a useful feature, but it could just as well be an independent package.

@tatchi
Copy link
Contributor

tatchi commented May 11, 2019

Hey @bryphe, @Akin909

I've been trying to implement something basic and would like to get your feedback. Some notes:

  • Currently the api is synchronous but I'm planning to switch to Lwt_io to make it async (which I guess would be better).
  • I'm using Hashtbl but was also looking at Map. From what I've read, the former is more performant but mutable (would Map or another structure be better ?)
  • haven't implemented yet the path detection based on the platform but will do that later on

Don't hesitate if you have any suggestions or if you would implement it differently.

/* PersistedStorage.rei */

let getItem: string => option(string);
let setItem: (string, string) => unit;
let removeItem: string => unit;
/* PersistedStorage.re */

let fileName = "filename";

let getFromFile = () => {
  switch (Pervasives.open_in(fileName)) {
  | input =>
    let hashtbl: Hashtbl.t(string, string) = Pervasives.input_value(input);
    Pervasives.close_in(input);
    hashtbl;
  | exception (Sys_error(message)) => Hashtbl.create(10)
  };
};

let saveToFile = (hashtbl: Hashtbl.t(string, string)): unit => {
  let output = Pervasives.open_out(fileName);
  Pervasives.output_value(output, hashtbl);
  Pervasives.close_out(output);
};

let getItem = (key: string): option(string) => {
  let hashtbl = getFromFile();
  switch (Hashtbl.find(hashtbl, key)) {
  | value => Some(value)
  | exception Not_found => None
  };
};

let removeItem = (key: string): unit => {
  let hashtbl = getFromFile();
  Hashtbl.remove(hashtbl, key);
  saveToFile(hashtbl);
};

let setItem = (key: string, value: string): unit => {
  let hashtbl = getFromFile();
  Hashtbl.replace(hashtbl, key, value);
  saveToFile(hashtbl);
};

@glennsl glennsl added A-other Area: Other, does not fit neatly into any of the other buckets enhancement New feature or request labels Nov 25, 2019
@leavengood
Copy link

Any update on this? The code above seems decent and would be useful for adding persistent sessions to Onivim2, which I was going to begin experimenting with this week.

@glennsl
Copy link
Member

glennsl commented Mar 8, 2020

I don't think a global singleton API is a good idea in general, because it makes the code that uses it impossible to test and hard to reason about. So I think the premise here is a bit flawed, but there's also a few other problems with that implementation:

  • The performance of Hashtbl vs Map is dwarfed by the fact that every call reads and/or writes from the filesystem. Ideally you'd read it lazily, ensure, or ensure it's reasonable to assume, that you're the only one writing to the file, and schedule writes for batching and to prevent blocking in hot paths.
  • input_value may raise exceptions that aren't handled. They should be.

Introducing asynchrony isn't going to solve much here. It'll just make the API harder to work with. Instead I think explicitly reading from file, having a handle that's passed around and scheduling writes would be a good trade-off between convenience, performance and referential transparency.

So an API more like this:

/* PersistedStorage.rei */

type t;

let load: (~filename) => t;
let get: (string, t) => option(string);
let set: (string, string, t) => unit;
let remove: (string, t) => unit;

@tatchi
Copy link
Contributor

tatchi commented Mar 8, 2020

Hey,

As per @lessp proposition on Discord, I just started a new repo with pretty much the same idea as what I described above (except that I now use Lwt to make things async). See: https://github.com/tatchi/re-localStorage/blob/master/library/LocalStorage.re

It's early work and I'm more than happy to gather feedback for the API design.

Thanks @glennsl for your suggestions, I agree that introducing async might make the API more complex. I also thought about having a kind of cached version kept in memory to avoid reading and/or writing to disk each time. I wanted to gather feedback and keep things simple before starting more complex work.

I'll start a new branch and try to implement what you proposed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-other Area: Other, does not fit neatly into any of the other buckets enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants