Skip to content

Commit bc88c58

Browse files
pilaodapilaoda
authored
fix Promise.prototype.finally to correctly propagate onFinally errors (#1725)
* (lualib): update Promise.prototype.finally to correctly handle onFinally errors and rejected * (test): add tests for async/await try-catch-finally re-throw cases
1 parent c40b6fa commit bc88c58

3 files changed

Lines changed: 147 additions & 12 deletions

File tree

‎src/lualib/Promise.ts‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -125,20 +125,17 @@ export class __TS__Promise<T> implements Promise<T> {
125125
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally
126126
// Delegates to .then() so that a new Promise is returned (per ES spec §27.2.5.3)
127127
// and the original fulfillment value / rejection reason is preserved.
128+
// reference: https://github.com/tc39/proposal-promise-finally/blob/fd934c0b42d59bf8d9446e737ba14d50a9067216/polyfill.js#L34-L41
128129
public finally(onFinally?: () => void): Promise<T> {
130+
if (typeof onFinally !== "function") {
131+
return this.then(onFinally, onFinally);
132+
}
129133
return this.then(
130-
onFinally
131-
? (value: T): T => {
132-
onFinally();
133-
return value;
134-
}
135-
: undefined,
136-
onFinally
137-
? (reason: any): never => {
138-
onFinally();
139-
throw reason;
140-
}
141-
: undefined
134+
x => new __TS__Promise(resolve => resolve(onFinally())).then(() => x),
135+
e =>
136+
new __TS__Promise(resolve => resolve(onFinally())).then(() => {
137+
throw e;
138+
})
142139
);
143140
}
144141

‎test/unit/builtins/async-await.spec.ts‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1126,6 +1126,104 @@ describe("try/catch in async function", () => {
11261126
.setTsHeader(promiseTestLib)
11271127
.expectToEqual(["finally", "ok"]);
11281128
});
1129+
1130+
test.each([0, 1, 2])("async re-throw (%p)", i => {
1131+
util.testModule`
1132+
const i: number = ${i};
1133+
async function foo() {
1134+
try {
1135+
try {
1136+
if (i === 0) { throw "z"; }
1137+
} catch (e) {
1138+
throw "a";
1139+
} finally {
1140+
if (i === 1) { throw "b"; }
1141+
}
1142+
} catch (e) {
1143+
throw (e as string).toUpperCase();
1144+
} finally {
1145+
throw "C";
1146+
}
1147+
}
1148+
export let result: string = "x";
1149+
async function run() {
1150+
try {
1151+
await foo();
1152+
} catch (e) {
1153+
result = (e as string)[(e as string).length - 1];
1154+
}
1155+
}
1156+
run();
1157+
`.expectToEqual({ result: "C" });
1158+
});
1159+
1160+
test("async: catch re-throws, finally still runs", () => {
1161+
util.testModule`
1162+
const foo = async () => {
1163+
throw "original";
1164+
};
1165+
1166+
let finallyCalled = false;
1167+
let caughtError: any = false;
1168+
1169+
const run = async () => {
1170+
try {
1171+
await foo();
1172+
} catch (e) {
1173+
throw "re-thrown: " + e;
1174+
} finally {
1175+
finallyCalled = true;
1176+
}
1177+
};
1178+
1179+
run().catch(e => { caughtError = e; });
1180+
1181+
export const result = { finallyCalled, caughtError };
1182+
`.expectToEqual({
1183+
result: {
1184+
finallyCalled: true,
1185+
caughtError: "re-thrown: original",
1186+
},
1187+
});
1188+
});
1189+
1190+
test("async: finally throw overrides catch throw", () => {
1191+
util.testModule`
1192+
const run = async () => {
1193+
try {
1194+
throw "try-error";
1195+
} catch (e) {
1196+
throw "catch-error";
1197+
} finally {
1198+
throw "finally-error";
1199+
}
1200+
};
1201+
1202+
let caughtError: any = false;
1203+
run().catch(e => { caughtError = e; });
1204+
1205+
export const result = caughtError;
1206+
`.expectToEqual({ result: "finally-error" });
1207+
});
1208+
1209+
test("async: finally return overrides catch throw", () => {
1210+
util.testFunction`
1211+
async function run() {
1212+
try {
1213+
throw "try-error";
1214+
} catch (e) {
1215+
throw "catch-error";
1216+
} finally {
1217+
return "finally-return";
1218+
}
1219+
}
1220+
1221+
let result: any;
1222+
run().then(v => { result = v; }).catch(e => { result = "rejected: " + e; });
1223+
1224+
return result;
1225+
`.expectToEqual("finally-return");
1226+
});
11291227
});
11301228

11311229
describe("async generators are unsupported", () => {

‎test/unit/builtins/promise.spec.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1370,4 +1370,44 @@ describe("Promise.finally", () => {
13701370
return result.value;
13711371
`.expectToEqual(99);
13721372
});
1373+
1374+
test("finally throw overrides fulfillment value", () => {
1375+
util.testModule`
1376+
let result: any;
1377+
Promise.resolve("ok")
1378+
.finally(() => { throw "finally-error"; })
1379+
.then(v => { result = v; }, e => { result = e; });
1380+
export const output = result;
1381+
`.expectToEqual({ output: "finally-error" });
1382+
});
1383+
1384+
test("finally throw overrides rejection reason", () => {
1385+
util.testModule`
1386+
let result: any;
1387+
Promise.reject("original")
1388+
.finally(() => { throw "finally-error"; })
1389+
.then(v => { result = v; }, e => { result = e; });
1390+
export const output = result;
1391+
`.expectToEqual({ output: "finally-error" });
1392+
});
1393+
1394+
test("finally returning rejected promise overrides fulfillment", () => {
1395+
util.testModule`
1396+
let result: any;
1397+
Promise.resolve("ok")
1398+
.finally(() => Promise.reject("finally-rejected") as any)
1399+
.then(v => { result = v; }, e => { result = e; });
1400+
export const output = result;
1401+
`.expectToEqual({ output: "finally-rejected" });
1402+
});
1403+
1404+
test("finally returning rejected promise overrides rejection", () => {
1405+
util.testModule`
1406+
let result: any;
1407+
Promise.reject("original")
1408+
.finally(() => Promise.reject("finally-rejected") as any)
1409+
.then(v => { result = v; }, e => { result = e; });
1410+
export const output = result;
1411+
`.expectToEqual({ output: "finally-rejected" });
1412+
});
13731413
});

0 commit comments

Comments
 (0)