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

Added Promise version for the storage API #105

Closed
wants to merge 1 commit into from
Closed

Added Promise version for the storage API #105

wants to merge 1 commit into from

Conversation

tanasebutcaru
Copy link

Hello guys,

I've been using this electron package for few weeks and I really needed a better looking syntax, so I've created a wrapper over the actual electron-json-storage API.

I thought it'll take too much to refactor the entire storage.js to replace callbacks with Promises, so I choose the fastest way of doing it.
The only downside of this solution is that every time the API will change (new methods, changes to current methods), the async.js file will have to be updated too!

Usage:

import * as storage from 'electron-json-storage/lib/async';

// Handle response in the Promise way
storage.get('foobar')
     .then(data => console.log(data))
     .catch(error => throw error);

// OR use `await`

async () => {
  try {
       const data = await.storage.get('foobar');
       console.log(data);
  } catch (error) {
       throw error;
  }
});

Note: everything works great, but the docs are not updated and there are no tests; consider this PR just a draft.

Let me know what do you think!
Would you merge this as is or would you like to refactor the storage.js instead?

Regards,
Tanase B.

@jviotti
Copy link
Member

jviotti commented Jun 19, 2018

Hi @tbutcaru ,

Thanks a lot for the PR! Is there a way we can modify the original functions so that they can be used both with promises and without promises (something like what Bluebird does)?

Maybe you can extend the utility function you wrote to support this, and then modify the original function definitions to use that instead?

I'd also love to see some tests ensuring that the promise interface works as expected.

@tanasebutcaru
Copy link
Author

Hi @jviotti,

That is doable, but it will definitely take some time.
I'll set a reminder for the next week and find some time to start doing that.

@tanasebutcaru
Copy link
Author

Closing this PR as I don't use this project anymore, hence no reason to work on it anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants