From 41ff9511d8eea43a9a2f577c8880ca48bb7e12bf Mon Sep 17 00:00:00 2001 From: Cold Fry Date: Thu, 26 Mar 2026 13:36:18 +0000 Subject: [PATCH 1/3] fix Promise.finally() to return new promise via .then() --- src/lualib/Promise.ts | 40 +++++++++++++----------------- test/unit/builtins/promise.spec.ts | 39 +++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/lualib/Promise.ts b/src/lualib/Promise.ts index c5b6aa2a5..187e5380d 100644 --- a/src/lualib/Promise.ts +++ b/src/lualib/Promise.ts @@ -45,7 +45,6 @@ export class __TS__Promise implements Promise { private fulfilledCallbacks: Array> = []; private rejectedCallbacks: PromiseReject[] = []; - private finallyCallbacks: Array<() => void> = []; // @ts-ignore public [Symbol.toStringTag]: string; // Required to implement interface, no output Lua @@ -124,16 +123,23 @@ export class __TS__Promise implements Promise { } // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/finally + // Delegates to .then() so that a new Promise is returned (per ES spec ยง27.2.5.3) + // and the original fulfillment value / rejection reason is preserved. public finally(onFinally?: () => void): Promise { - if (onFinally) { - this.finallyCallbacks.push(onFinally); - - if (this.state !== PromiseState.Pending) { - // If promise already resolved or rejected, immediately fire finally callback - onFinally(); - } - } - return this; + return this.then( + onFinally + ? (value: T): T => { + onFinally(); + return value; + } + : undefined, + onFinally + ? (reason: any): never => { + onFinally(); + throw reason; + } + : undefined + ); } private resolve(value: T | PromiseLike): void { @@ -168,25 +174,13 @@ export class __TS__Promise implements Promise { private invokeCallbacks(callbacks: ReadonlyArray<(value: T) => void>, value: T): void { const callbacksLength = callbacks.length; - const finallyCallbacks = this.finallyCallbacks; - const finallyCallbacksLength = finallyCallbacks.length; if (callbacksLength !== 0) { for (const i of $range(1, callbacksLength - 1)) { callbacks[i - 1](value); } // Tail call optimization for a common case. - if (finallyCallbacksLength === 0) { - return callbacks[callbacksLength - 1](value); - } - callbacks[callbacksLength - 1](value); - } - - if (finallyCallbacksLength !== 0) { - for (const i of $range(1, finallyCallbacksLength - 1)) { - finallyCallbacks[i - 1](); - } - return finallyCallbacks[finallyCallbacksLength - 1](); + return callbacks[callbacksLength - 1](value); } } diff --git a/test/unit/builtins/promise.spec.ts b/test/unit/builtins/promise.spec.ts index cb18db994..8dd751723 100644 --- a/test/unit/builtins/promise.spec.ts +++ b/test/unit/builtins/promise.spec.ts @@ -1323,3 +1323,42 @@ describe("Promise.race", () => { }); }); }); + +// https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1659 +describe("Promise.finally returns new promise", () => { + test("finally returns a different promise instance", () => { + util.testFunction` + const p1 = new Promise(() => {}); + const p2 = p1.finally(); + return p1 === p2; + `.expectToEqual(false); + }); + + test("finally preserves fulfillment value", () => { + util.testFunction` + let result: any; + Promise.resolve(42).finally(() => {}).then(v => { result = v; }); + return result; + `.expectToEqual(42); + }); + + test("finally preserves rejection reason", () => { + util.testFunction` + let result: any; + Promise.reject("error").finally(() => {}).catch(r => { result = r; }); + return result; + `.expectToEqual("error"); + }); + + test("finally on deferred rejection preserves reason", () => { + util.testFunction` + const { promise, reject } = defer(); + let result: any; + promise.finally(() => {}).catch(r => { result = r; }); + reject("deferred error"); + return result; + ` + .setTsHeader(promiseTestLib) + .expectToEqual("deferred error"); + }); +}); From bbb3ab3c738e088f12f17dfcb301bd77fbef2106 Mon Sep 17 00:00:00 2001 From: Cold Fry Date: Fri, 27 Mar 2026 10:28:06 +0000 Subject: [PATCH 2/3] simplify Promise.finally test to use expectToMatchJsResult Remove redundant tests that relied on synchronous polyfill behavior differing from JS. Keep only the identity check which matches JS. --- test/unit/builtins/promise.spec.ts | 42 +++++------------------------- 1 file changed, 6 insertions(+), 36 deletions(-) diff --git a/test/unit/builtins/promise.spec.ts b/test/unit/builtins/promise.spec.ts index 8dd751723..c03ad98a1 100644 --- a/test/unit/builtins/promise.spec.ts +++ b/test/unit/builtins/promise.spec.ts @@ -1325,40 +1325,10 @@ describe("Promise.race", () => { }); // https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1659 -describe("Promise.finally returns new promise", () => { - test("finally returns a different promise instance", () => { - util.testFunction` - const p1 = new Promise(() => {}); - const p2 = p1.finally(); - return p1 === p2; - `.expectToEqual(false); - }); - - test("finally preserves fulfillment value", () => { - util.testFunction` - let result: any; - Promise.resolve(42).finally(() => {}).then(v => { result = v; }); - return result; - `.expectToEqual(42); - }); - - test("finally preserves rejection reason", () => { - util.testFunction` - let result: any; - Promise.reject("error").finally(() => {}).catch(r => { result = r; }); - return result; - `.expectToEqual("error"); - }); - - test("finally on deferred rejection preserves reason", () => { - util.testFunction` - const { promise, reject } = defer(); - let result: any; - promise.finally(() => {}).catch(r => { result = r; }); - reject("deferred error"); - return result; - ` - .setTsHeader(promiseTestLib) - .expectToEqual("deferred error"); - }); +test("finally returns a different promise instance", () => { + util.testFunction` + const p1 = new Promise(() => {}); + const p2 = p1.finally(); + return p1 === p2; + `.expectToMatchJsResult(); }); From bbc4fee4aceb928c8c8e02f076e3f3ea72b5f323 Mon Sep 17 00:00:00 2001 From: Cold Fry Date: Fri, 27 Mar 2026 17:33:53 +0000 Subject: [PATCH 3/3] add tests for Promise.finally value preservation and rejection passthrough Fix issue reference (#1660, not #1659). --- test/unit/builtins/promise.spec.ts | 53 ++++++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/test/unit/builtins/promise.spec.ts b/test/unit/builtins/promise.spec.ts index c03ad98a1..f323b030f 100644 --- a/test/unit/builtins/promise.spec.ts +++ b/test/unit/builtins/promise.spec.ts @@ -1324,11 +1324,50 @@ describe("Promise.race", () => { }); }); -// https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1659 -test("finally returns a different promise instance", () => { - util.testFunction` - const p1 = new Promise(() => {}); - const p2 = p1.finally(); - return p1 === p2; - `.expectToMatchJsResult(); +// https://github.com/TypeScriptToLua/TypeScriptToLua/issues/1660 +describe("Promise.finally", () => { + test("returns a different promise instance", () => { + util.testFunction` + const p1 = new Promise(() => {}); + const p2 = p1.finally(); + return p1 === p2; + `.expectToMatchJsResult(); + }); + + test("preserves fulfillment value", () => { + util.testFunction` + const result = Promise.resolve(42).finally(() => {}) as any; + return result.value; + `.expectToEqual(42); + }); + + test("preserves rejection reason", () => { + util.testFunction` + const result = Promise.reject("err").finally(() => {}) as any; + return result.rejectionReason; + `.expectToEqual("err"); + }); + + test("callback executes on fulfillment", () => { + util.testFunction` + let called = false; + Promise.resolve(1).finally(() => { called = true; }); + return called; + `.expectToEqual(true); + }); + + test("callback executes on rejection", () => { + util.testFunction` + let called = false; + Promise.reject("err").finally(() => { called = true; }); + return called; + `.expectToEqual(true); + }); + + test("finally with undefined callback", () => { + util.testFunction` + const result = Promise.resolve(99).finally(undefined) as any; + return result.value; + `.expectToEqual(99); + }); });