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.

[ljharb]

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