Additional .then call incorrectly inserted

[wiktor-k]

Hello,

I've encountered a case when kneden incorrectly adds extra .then(function () {}); call at the end of the generated async function.

Consider this code:

async function handler() {
    const response = await fetch('http://address');
    if (!response.ok) {
        return null; // 1
    }
    const json = await response.json(); // 2
    return {
      a: 3
    };
}

It produces the following code:

'use strict';

function handler() {
    var response, json;
    return Promise.resolve().then(function () {
        return fetch('http://address');
    }).then(function (_resp) {
        response = _resp;

        if (!response.ok) {
            return null; // 1
        } else {
            return Promise.resolve().then(function () {
                return response.json();
            }).then(function (_resp) {
                json = _resp; // 2

                return {
                    a: 3
                };
            });
        }
    }).then(function () {});
}

The last .then call swallows the last return statement. If you comment out line 1 or 2 then the generated code is correct (no extra .then).

I'm using:
require('babel-core').transform(source, { presets: "es2015", plugins: ["kneden"]}).code
with these dependencies:

  "dependencies": {
    "babel-core": "^6.5.2",
    "babel-preset-es2015": "^6.5.0",
    "kneden": "^1.0.1"
  }

Oh, by the way thanks for this awesome library! The generated code is perfectly readable.

[marten-de-vries]

Nice find, returns are tricky... If statements in particular are rewritten in all kinds of ways to try to prevent this kind of situation. Remove 1 or 2, and other code paths will be taken so that doesn't surprise me. Can I use your input sample as the base of a test suite test case?

The underlying problem is that if statements, at least in some situations, are probably marked as 'dirty': that means, they can return other values than actual return values, e.g. when await-ing something. That's wrong: instead, if statement branches (the code inside if and else blocks) should be marked as possibly dirty, but not the whole thing.

[nolanlawson]

@marten-de-vries Hmm, if you convert an empty return to return undefined and then explicitly return undefined from inside the Promise chain, could you remove the .then(function () {}) entirely?

[marten-de-vries]

.then(function () {}) is for the following scenario:

async function test() {
  await a();
}

->

function test() {
  return Promise.resolve().then(function () {
    return a();
  }).then(function () {});
}

So, no. Adding a return statement in that case is only more verbose. The problem is that the final part of the chain is added while it really shouldn't be for this issue.

[wiktor-k]

@marten-de-vries sure, use this example as you want. I tried to make it as simple as possible. I've got a lot of async code that needs to run on ES5 so when a new version of kneden is available I can thoroughly test it :)

Currently I'm using fast-async but kneden's output looks a lot better.

[nolanlawson]

@marten-de-vries Thanks, make sense.

[marten-de-vries]

Added failing test in 89f3a91.

[marten-de-vries]

#43 contains a fix. Needs more testing, though.

[mnpenner]

As a side-note, couldn't

const response = await fetch('http://address');

By translated into

fetch('http://address').then(function(response) { ... })

Instead? Or if you're worried about fetch or other such functions not being fully A+ compliant, you could do:

Promise.resolve(fetch('http://address')).then(function(response) { ... })

Couldn't you? That would be a little more compact and legible than

return Promise.resolve().then(function () {
    return fetch('http://address');
}).then(function(response) { ... })

IMO.

[ljharb]

To be spec compliant, the entire body of the async function runs synchronously until the first await, throw, or return. However, it can't know if fetch returns a thenable or not (function fetch() { return 3 } for example). Realistically it should use return new Promise(resolve => { resolve(fetch(…)).then(response => { … }) to get the "synchronous but catches errors and handles thenables" semantics.

[mnpenner]

Aha. Sorry for bothering you, I thought I saw an opportunity for improvement but I should have known better 😄

[raphaelokon]

I forked this repo and did some tests locally.

// 1. AwaitExpression
async function fnAwaitExpression(a){
  if(!a) return;
  await Promise.resolve(a);
}
// 2. AwaitExpression (transpiled)
function _fnAwaitExpression(a) {
    return Promise.resolve().then(function () {
        if (!!a) {
            return Promise.resolve(a);
        }
    }).then(function () {});
}

// 3. ReturnExpression + AwaitExpression
async function fnReturnAwaitExpression(a){
  if(!a) return;  
  return await Promise.resolve(a);
}
// 4. ReturnExpression + AwaitExpression (transpiled)
function _fnReturnAwaitExpression(a) {
    return Promise.resolve()
    .then(function () {
        if (!!a) {
            return Promise.resolve(a);
        }
    })
    // We know this swallows the inner return. 
    // And we know this is added for the case of an AwaitExpression, @see 1. and 2. 
    .then(function () {}); 
}

fnAwaitExpression("a").then(v => {console.log(v)});   //=> nothing!
_fnAwaitExpression("a").then(v => {console.log(v)});  //=> nothing! Correct behavior.


fnReturnAwaitExpression("b").then(v => {console.log(v)});   //=> b
_fnReturnAwaitExpression("b").then(v => {console.log(v)});  //=> nothing! Wrong behavior. Expected b.

In order to add the additional .then(function () {}); in a non breaking way and still swallowing the return in case of non-returned AwaitExpression (1) , the .then(function () {}); could be appended directly to the Promise-yielding function to stay functionally equal (not returning anything):

function _fnAwaitExpression(a) {
    return Promise.resolve().then(function () {
        if (!!a) {
            return Promise.resolve(a)
+                .then(function () {});
        }
    })
-   .then(function () {});
}

So moving the then would mean that in the case of a returning AwaitExpression (3) or a ReturnExpression followed by an AwaitExpression, the additional then could be omitted, resulting in correct behavior:

function _fnAwaitExpression(a) {
    return Promise.resolve().then(function () {
        if (!!a) {
            return Promise.resolve(a)
        }
    });
-   .then(function () {});
}

_fnReturnAwaitExpression("b").then(v => {console.log(v)});  //=> b! Correct behavior.

[raphaelokon]

A small test case …

[yejinjian]

@raphaelokon @marten-de-vries
you can do like this:

async function fnAwaitExpression(a){
	if(!a) return;
	const ret = await Promise.resolve(a);
	return ret;
}

result:

function fnAwaitExpression(a) {
	var ret;
	return Promise.resolve().then(function () {
		if (!!a) {
			return Promise.resolve().then(function () {
				return Promise.resolve(a);
			}).then(function (_resp) {
				ret = _resp;

				return ret;
			});
		}
	}).then(function () {});
}

some times i should do something after await and brefore return, But ...

[ljharb]

I don't think this is resolved yet.

[wiktor-k]

Seeing This project is currently unmaintained. in the README I closed it as it will likely never be resolved.

[ljharb]

That may be true, but that means that the issue should remain open forever, in case someone comes along later to maintain it.