Additional .then call incorrectly inserted about kneden HOT 16 OPEN

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.

from kneden.

@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?

from kneden.

.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.

from kneden.

@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.

from kneden.

@marten-de-vries Thanks, make sense.

from kneden.

Added failing test in 89f3a91.

from kneden.

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

from kneden.

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.

from kneden.

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.

from kneden.

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

from kneden.

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.

from kneden.

A small test case …

from kneden.

@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 ...

from kneden.

I don't think this is resolved yet.

from kneden.

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

from kneden.

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

from kneden.