electron-userland / electron-json-storage

:package: Easily write and read user settings in Electron apps
1.44k stars 80 forks source link

Promise support? #73

Open Rob-- opened 7 years ago

Rob-- commented 7 years ago

This is a really useful library and I think it could be improved if there was support for promises. It would make the library easier to use, for example:

try {
    if (await storage.has(username)) {
      return await storage.get(username);
    }
    return false;
} catch(e) {
    return false;
}
jviotti commented 7 years ago

Hey there, thanks for reaching out.

We used to have promises support at some point using Bluebird, however we dropped them given some nasty issues with ES6 promises (see https://github.com/electron-userland/electron-json-storage/issues/5).

Is promisifying the module using Bluebird an option for you? For example:

const Bluebird = require('bluebird');
const storage = Bluebird.promisifyAll(require('electron-json-storage'));

storage.getAsync('foo').then((data) => {
    ...
});
Rob-- commented 7 years ago

Why use Bluebird? Why not use native promises? This would also remove all the async.waterfall and async.parallel code which not only (in my opinion) looks ugly but also makes the code very hard to read.

For example, the .get function would look like this:

exports.get = (key, callback) => {
  const fileName = utils.getFileName(key);

  return new Promise((resolve, reject) => {
    fs.readFile(fileName, { encoding: 'utf-8' }, (error, object) => {
      if (error) {
        reject(error);
        return;
      }

      try {
        const json = error.code === 'ENOENT' ? {} : object;
        resolve(JSON.parse(json));
      } catch (error) {
        reject(new Error(`Invalid data: ${object}`));
      }
    });
  });
};

Example usage:

async function getDetails(username) {
  const data = await storage.get(username);
  return decrypt(data);
}

// or

function getDetails(username, callback) {
  storage.get(username).then((data) => {
    callback(decrypt(data));
  });
}

Another suggestion is to use ES6 features, Node.js long term support is now active for Node 6.x which supports ES6. Also notice how the use of native promises shortens the code and makes it incredibly easier to read as well .

jviotti commented 7 years ago

I'm happy to slowly transition to native promises :+1:

Jack-Barry commented 5 years ago

Any updates on this one? Would love to use async/await with these methods

Jack-Barry commented 5 years ago

Did find a workaround for this - didn't want to go so far as adding Bluebird so just did a manual Promise for it:

async function getData() {
  return new Promise((res, rej) => {
    storage.get('some_key', (err, data) => {
      if (err) rej(err)
      res(data)
    }
  }
}

// Somewhere else
const data: ISomeInterface = (await getData()) as ISomeInterface

if (typeof data !== 'undefined') {
  // do stuff
}

Taken a step further, mocking this in Jest:

let mockData: jest.Mock = jest.fn()

jest.mock('electron-json-storage', () => {
  return {
    get: (key: string, callback: Function): void => callback('', mockData())
  }
})

describe('my thing', () => {
  it('does stuff after getting data', async () => {
    mockData.mockImplementationOnce(() => ({
      // some data
    }))
    const myThingData = await MyThing.getData()
    expect(myThingData).toEqual(// some data)
  })
})
jviotti commented 5 years ago

@Jack-Barry We initially had promise support through Bluebird, but we faced some inter-operatibility issues between Bluebird and Electron that made us go back to plain callbacks. Native promises will probably do, though.

I think we could start slowly transitioning to native Promises by adding them in all the methods while still preserving the callback interface. I might be able to work on it soon, but otherwise we're happy to take PRs

Jack-Barry commented 5 years ago

@jviotti Would love to contribute! I figured they'd need to be implemented in a way that wouldn't be a breaking change. Might take me a few weeks to get around to it but will take a look if it's not implemented by the time I can.

Cheers 🍺

mdodge-ecgrow commented 1 year ago

Any updates on this?

I'm trying to figure out how to use this correctly without async/await.

let environments = [];
    storage.get('SADT-environtments', function(error, data) {
      if (error) throw error;
      console.log({data})
      environments = JSON.parse(data.environments);
    });
    console.log({ environments });

environments is always an empty array. How can I fix that?

Update: Scratch that last question about fixing the empty array. I implemented a method like @Jack-Barry did to promisify the function. I had to do it with .has as well. Would love to be able to just use async/await without any extra work.

jviotti commented 1 year ago

Hey there! Yeah, promising my recommendation too!