Support for synchronous operations

[philippefutureboy]

Hi!

First of all, thank you for the good work, works like a charm! 🚀

I would like to know if it would be possible to provide a sync implementation of the API. Indeed, in cases where the JSON storage data is used to instantiate modules, having to wrap the instantiation with a Promise/callback is less than optimal. For instance, in my case I am receiving database connection configuration from the user, and I store it locally. I can then use this to instantiate my database by default on the subsequent boots of the app.

This would be a really useful :D

Have a wonderful day,

Cheers,

Philippe

[jviotti]

Hi @philippefutureboy, thanks for the nice words! I'm definitely open to a PR that adds the sync functions. Do you think there is a way we can provide both without much duplication in the internal implementation?

[philippefutureboy]

Nice! I'll give it a look next week to see how I can contribute.

After a quick readthrough, my suggestion would be to include deepmerge as well as fs-extra in your dependencies, and to wrap all of your functions with another function that has the same signature, but that merges a 'sync' option parameter into your option object, as follows:

const fs = require('fs-extra');

exports.get = function(key, options, callback) {
...
  if(typeof options === 'object' && typeof callback === 'undefined' && options.sync) {
    ...
    return fs.readJSONSync(...);
  } else {
    ...
    return fs.readJSON(...);
  } 
}

exports.getSync = function(key, options) {
  return get(key, deepmerge(options, {sync: true}));
}

I'm sure this could reduce the duplication of code :)

[philippefutureboy]

~ weeks later ~

So I have more time on hand now, I can contribute. I was thinking to rehaul the entire library to use fs-extra and Promises. Would that be something you'd be interested in?

[jviotti]

@philippefutureboy

I was thinking to rehaul the entire library to use fs-extra and Promises. Would that be something you'd be interested in?

I'd prefer keeping the list of dependencies to a minimum. There is an existing PR for adding promise support: #105.

After a quick readthrough, my suggestion would be to include deepmerge as well as fs-extra in your dependencies, and to wrap all of your functions with another function that has the same signature, but that merges a 'sync' option parameter into your option object, as follows:

Lets give that a shot, and see how it goes, as it sounds very cool :) Regarding deepmerge, we can probably get away with Object.assign()

[philippefutureboy]

Wait, so do you want fs-extra or not? Oo

Object.assign does shallow merging, whereas deepmerge does deep merging. If we need merging, it most likely be deep merging.

What do you think?

[jviotti]

We're not dealing with objects that require deep merging, right? (the ones in the example above are one level, so Object.assign should work).

Regarding fs-extra, what particular functions from that library are you trying to use?

[philippefutureboy]

True! Sorry, I realized I hadn't communicated the changes that I had thought about since we last discussed.

Essentially what I realized is that a lot of the logic in your package can be replaced with fs-extra's readJSON and readJSONSync, coupled with promise support. Correct me if I'm missing something, but my understanding is that electron-json-storage is a key-value store that fetches JSON files as the values for each key, with the added benefit of checking for RW locks. If it is the case, then the library could be rewritten to a single file utility with >= 100 lines with the use of readJSON.

What do you think?

[jviotti]

@philippefutureboy I see what you mean now. Yeah, lets give it a shot, and see how the result looks like. If the amount of code is reduced, then that's a big win, and will make it easier to then refactor the project to offer both sync and async versions of the functions.

[philippefutureboy]

@jviotti Wonderful! I'll give it a look this weekend

[philippefutureboy]

@jviotti I've given it some time this weekend – Started with a refresher of the tech used – moving to babel & eslint => see here

I'm having a series of test failures, I'll have to look into it as soon as I have some time on hand.

[philippefutureboy]

@jviotti I haven't forgotten this. I'm simply overwhelmed with work. I'll definitely get back to this at some point cause I want to see this through.

I also had an idea that I implemented on my side and that works wonders – I wrapped the storage module with an EventEmitter pattern, which grants me the ability to subscribe services to changes in state. This, in turn, allows to get state always synchronously (exception being the initialization). This is because the get state function on my wrapper only retrieve cached state, and updates the cached state on set.

Anywho, we'll discuss more in details when I actually get some time 😅

Have a wonderful day,

Cheers,

Philippe