Use Promises internally

[brettz9]

Use Promises internally (avoids need for dep. and prepares for readFilesystemTree deprecating callback API)

The Promises and async/await are supported in the maintained Node versions (8+).

Btw, thanks for the commit access. Would you like me to submit any changes as PRs, or directly commit chores like updating non-breaking dependencies, lint fixes, etc.?

[brettz9]

Also, are you ok with the preferring const (or if changeable let) style, and other ES6 features like destructuring, etc.?

[kemitchell]

@brettz9 Please open PRs for any meaningful changes, and try to get someone else to review and feed back, even if that takes time.

I would strongly prefer to stick to ES5 and callbacks, and not to change anything just for the sake of change.

But I am all for paving a path for Promise preferring programmers. Perhaps we could document manual and library-based .promisify() in README?

[ljharb]

https://npmjs.com/util.promisify can be used to support back to node 0.12, or a Promise-polyfilled node 0.4. In newer nodes with util.promisify, it uses that implementation instead.

[kemitchell]

@ljharb, thanks! I knew there was something like that in core.

I'd be all for adding example with util.promisify to README.

[brettz9]

This PR though isn't for returning a Promise, as I know you said you didn't want to add that complexity; this PR actually just eliminates a dependency (and happens to also prepare for a change planned in readFilesystemTree to move to Promises only).

As far as ES6, I tend to find those features make things more succinct and enables me to detect bugs. Are you wishing to enforce ES5 for new features too or are you only against refactoring old code when not needed?

[kemitchell]

@brettz9 readFilesystemTree is defined in this package. Did you mean read-package-tree? Could you link me to a PR or issue for that planned change?

[ljharb]

Using async function blocks older nodes from using it; you can make the function return a Promise directly which works down to node 0.12.

[brettz9]

Yes, sorry, read-package-tree.

Here is the source for that statement: https://github.com/npm/read-package-tree/blob/master/rpt.js#L145 , though I see now it is a little tentative in possibly dropping the callback by saying "and/or".

[brettz9]

@ljharb : Sure, but didn't you move to only supporting maintained Node versions anyways? It is covered by those versions.

[brettz9]

@ljharb : Also, as it is currently, the use of array extras like find won't get Node support that far back.

[ljharb]

Fair, but anything that's API can be polyfilled (NODE_OPTIONS='--require=es5-shim --require=es6-shim' licensee, for example) while syntax can't without transpilation.

[brettz9]

It seems to me that if users are going to have to go through hoops to use older code, and we aren't testing in CI to ensure they still can, they can try to add transpilation as well. There is value for users too, I think, in making things easier on developers. But sure I can amend that if that is desired.

[brettz9]

We're also not using the likes of https://github.com/mysticatea/eslint-plugin-node/blob/master/docs/rules/no-unsupported-features/es-syntax.md to ensure that we don't already have some such features like arrow functions anyways which aren't supported by those older Node versions.

[kemitchell]

I will ask Isaac about read-package-tree. Eventually SemVer will tell us, either way.

In general, I'm not against any particular syntax or API. I am instead strongly in favor of economy of motion.

If and when an important dependency forces a change, this package will have to adapt. But until then, quiet and simple keep folks like me around. Feature and doc improvements are welcome. I have at least one feature in mind myself. But if this repo starts to churn, I'm going to have to step away from it.

[brettz9]

Good to know, and we definitely want you around! :)

Off topic, but just to mention, I made a small commit which I think should be non-controversial in merely ensuring that null is not passed in.

[kemitchell]

@brettz9 please open PRs for changes. I want to see them, it helps prevent master contention, and it gets my attention so I can decide whether to publish a new version.

[brettz9]

Ok, will do. Lot of potential edge cases, so now I know to err on the side of caution.

[ronkorving]

Would it not make more sense to get rid of the callback altogether then? This looks a bit like a function with an identity crisis, returning both a promise and sending errors to a callback instead of rejecting the promise :)

[brettz9]

Yes. But since there would indeed be a little more work to support both callback and Promise returning, I was trying to honor @kemitchell 's preference for not adding complexity in supporting both callback and Promise; I thus confined myself here to those internal Promise additions which were strictly necessary for removing the dependency.

But if y'all are open to a breaking change, then yes, a public Promise API would make this change cleaner as well (and of course easier for users who wouldn't need to clutter their code with extra promisify statements).

[ronkorving]

My bad. To clarify, there is no "y'all" here :) This is really @kemitchell's project, and I'm an interested bystander. Let me give this review another shot.

[ronkorving]

There's still the problem that errors will not find their way to the promise.

How about keeping the function as-is, but wrapping it with a function that takes care of the promisification?

Something like this:

function licensee(configuration, path, callback) {
  if (arguments.length < 3) {
    return new Promise(function (resolve, reject) {
      realLicensee(configuration, path, function (error, result) {
        if (error) {
          reject(error);
        } else {
          resolve(result);
        }
      });
    });
  } else {
    realLicensee(configuration, path, callback);
  }
}

Then you have both patterns without having to change the licensee logic at all.

[brettz9]

I'm indeed happier with that and would have done that, as I'd like to see a built-in Promise if that's ok with @kemitchell , but I've gotten the impression this whole PR may only end up merged in some form if it's needed by read-package-tree per #62 (comment) and even then, it sounds like, per #62 (comment) , that promisify is going to be expected rather than having the project providing its own Promise-returning ability .

The way I implemented tries to meet this apparent expectation, as it still works in a backward compatible way even if it doesn't fully take advantage of Promises so that the public API allows it (including sending Promise users rejections upon errors) as in your sample.

[kemitchell]

This PR is indeed on hold. See #64.

[ljharb]

We now require node 14+, so we can use any syntax in that version as well as util.promisify.