ethereumjs / keythereum

Create, import and export Ethereum keys
MIT License
609 stars 163 forks source link

Add support for async/await in addition to callbacks #58

Closed aleybovich closed 1 week ago

aleybovich commented 6 years ago

Currently, I have to write my own await/async wrapper for the async functions in the library. Would it be possible to promisify those async functions (e.g. exportToFile, importFromFile)? It could be using the fairly standard pattern - if cb is specified, then use callback; otherwise return a promise.

tinybike commented 6 years ago

Yes, it's definitely possible, I just haven't gotten around to doing it! A PR implementing this would be welcome, though, if you feel like implementing it @aleybovich :)

chikeichan commented 5 years ago

specifying cb is already used to define whether the function is invoked synchronously or asynchronously.

Changing it to following the standard cb pattern to return promises would introduce a breaking changes to previous version.

I think we have a few options:

@tinybike @aleybovich thoughts?

aleybovich commented 5 years ago

@chikeichan I think you can introduce async/await (or really just promises because async/await is just syntax sugar on top of that) without breaking downward compatibility.

Currently, you have two ways of invoking a function:

func(...args, cb) - executes asynchronously, doesn't return anything and fires cb when ready func(...args) - executes synchronously, returns the result

Now, you could make a small and compatible change to the async version - if we pass true instead of a cb, treat that function as async that should return a promise. Not many changes will be required - basically if true is passed instead of a callback function, you instantiate a promise and resolve/reject when you get results instead of firing a cb.

This will ensure the backwards compatibility and you will avoid the need to introduce new namespaces and/or functions.

aleybovich commented 5 years ago

So for example, exportToFIle will become something like this:

exportToFile = function (keyObject, keystore, cb) {
    var outfile, outpath, json, fs;
    keystore = keystore || "keystore";
    outfile = this.generateKeystoreFilename(keyObject.address);
    json = JSON.stringify(keyObject);

    // If cb is a bool `true`, this function will return a promise
    const isAsyncPattern = cb === true;

    if (this.browser) {
      if (!isFunction(cb)) return json;
      // Return a resolved promise if async pattern requested
      else if (isAsyncPattern) return Promise.resolve(json);
      return cb(json);
    }
    outpath = require("path").join(keystore, outfile);
    fs = require("fs");
    if (!isFunction(cb)) {
      fs.writeFileSync(outpath, json);
      return outpath;
    }
    if (isAsyncPattern) {
        return new Promise((resolve, reject) => {
            fs.writeFile(outpath, json, function (err) {
                if (err) reject(err);
                resolve(outpath);
              });
        });
    }
    fs.writeFile(outpath, json, function (err) {
      if (err) return cb(err);
      cb(outpath);
    });
  };
chikeichan commented 5 years ago

@aleybovich That works too! I will go with this approach