stevenvachon / winattr

Foolproof Windows® file attributes.
MIT License
24 stars 6 forks source link

promisify the async functions #12

Open aminya opened 4 years ago

aminya commented 4 years ago

Closes #9

If you want this:

Install the package:

npm install @aminya/winattr
aminya commented 4 years ago

@stevenvachon Could you check this?

stevenvachon commented 4 years ago

Some tests for the promises, please.

aminya commented 4 years ago

@stevenvachon I added the tests, types, and documentation. Note that this should be a major release due to changing the API.

stevenvachon commented 4 years ago

Oh, I thought this added promises, but you actually removed the callback. I'd prefer it if it supported both. But if it's better to drop callbacks, I think it's better to write as promises instead of wrapping with util.promisify.

aminya commented 4 years ago

Oh, I thought this added promises, but you actually removed the callback. I'd prefer it if it supported both. But if it's better to drop callbacks, I think it's better to write as promises instead of wrapping with util.promisify.

Promisify automatically removes callbacks and adds promises. This way we don't need to change all the code. So, I have added promises here.

If you want, we can export _get, _set to support the old behavior.

stevenvachon commented 4 years ago

I'd rather have a function that accepts a callback and returns a promise.

aminya commented 4 years ago

I'd rather have a function that accepts a callback and returns a promise.

What's the point of returning a Promise when we have callbacks? The whole point of Promise is to await it any time we like and run our callback anytime we want. Callbacks should be avoided, that's old JavaScriptism which has ruined a lot of codebases. That's why Node made promisify.

http://callbackhell.com/

stevenvachon commented 4 years ago

Not everyone uses promises. The fs lib still defaults to callbacks too and this lib was created to resemble core Node.js modules.

aminya commented 4 years ago

Not everyone uses promises. The fs lib still defaults to callbacks too and this lib was created to resemble core Node.js modules.

Those that don't use callback can always use then(()=>{}), but the opposite is not possible. You can't await a non-promisified callback-based function.

The only reason I made this PR was to remove callbacks. If this is this library wants to go towards this path, I prefer to make a new promisified library that ditches callbacks.

stevenvachon commented 4 years ago
const func = callback => new Promise((resolve, reject) => {
  _func((error, result) => {
    if (callback) {
      callback(error, result);
    } else if (error != null) {
      reject(error);
    } else {
      resolve(result);
    }
  });
};

func((error, result) => {});
func().then(result => {});
aminya commented 3 years ago

Anyone who wants this PR, I have registered it under a new name:

npm install @aminya/winattr