fix: replace pify with simpler promise helpers

[filipesilva]

pify seems to not be working correctly when a decorated file system is used as inputFileSystem.

Fix #217

[michael-ciniawsky]

src/utils/promisify.js

export const stat = ...
export const readFile = ...

import { stat, readFile } from './utils/promisify';

[filipesilva]

On it.

[filipesilva]

Done.

[alexander-akait]

Thanks for help!

[michael-ciniawsky]

@filipesilva Thx

[jcyovera]

Thank you for the fix!

[kabe5965]

New version 4.4.1 did it! Thanks! Deploy to Heroku works as well.

[filipesilva]

@evilebottnawi looked a bit more into why pity was not working with the decorated file system. It turns out pify uses for (const key in obj) { } to iterate over the object, but the decorated file system function were not enumerable and thus did not show up there.

For the decorated file system in Angular CLI we used a TypeScript class, which downlevels to a ES6 class. It seems pify does not work with ES6 class instances. I opened an issue for it in sindresorhus/pify#57.

[sindresorhus]

I don't understand why the fix is so overcomplicated though. You could have just used pify on the individual methods.

[filipesilva]

@sindresorhus in hindsight, that would have been a great solution. At the time I did not know only one of the pify modes of operation had problems with ES6 classes.

We were looking at getting a fix in since it was affecting a lot of users in angular/angular-cli#9550. After fixing the issue proper I went to report the issue on pify with a repro and found out that the other usage mode was ok.

[ronkorving]

Without an else or early return statement, resolve will even be called in the case of a rejection. Same is the case for readFile below.

[alexander-akait]

@ronkorving reject already do early exit, no necessary return

[ronkorving]

How can it? To my knowledge, the only way to return early in JS is return or throw.
reject(err) is just a function call.

[alexander-akait]

@ronkorving reject() === throw inside promise

[filipesilva]

It is true that resolve(stats) will still be called, but the stats value will not be passed on to anything. Even if there is another promise waiting for the resolve value, the rejection path will be taken instead.

It's a still a bug though, in the sense that it was never intended for the resolve to run in the first place.

@evilebottnawi WDYT of me doing another PR that adds back pify with the alternative syntax that works on ES6 classes?

[alexander-akait]

@ronkorving IMHO, return here need only for easy reading and understanding code. Feel free to PR if this confusing for you

[ronkorving]

@evilebottnawi I don't think I'm confused. As @filipesilva also pointed out, resolve(stats) will still be called.

I'll leave it to you both to figure out the way forward, given that there seems to be a desire to put pify back in anyway.

[filipesilva]

I tried #228 but just using pify on the single method doesn't seem to work well either, this seems to be undefined when doing that so class members aren't accessible.

@ronkorving would you like to make a PR to change the if to a if ... else?

[michael-ciniawsky]

if (err) reject(err) // I'm not 100% here, but this should be === (x) => x (return)

resolve(x)

if (err) {
   return reject(err)
}

resolve(x)

if (err) {
   reject(err)
   // eventually to guard against 'runaway' promises
   return null
}

resolve(x)

Please no extra lib, it's not necessary to promisify two callback functions :)

[michael-ciniawsky]

@ronkorving Thx. Nice catch :)

[ronkorving]

I noticed what I believe to be a bug in this PR and left a comment.