kneden
kneden copied to clipboard
babel
Additional .then call incorrectly inserted
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.
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.
@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?
.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.
@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.
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.
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.
Aha. Sorry for bothering you, I thought I saw an opportunity for improvement but I should have known better :smile:
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 @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 ...
Seeing This project is currently unmaintained. in the README I closed it as it will likely never be resolved.
That may be true, but that means that the issue should remain open forever, in case someone comes along later to maintain it.