Change behavior of finally

Closes #59
Closes #66

lib/init.lua

@@ -1205,6 +1205,14 @@ end
 function Promise.prototype:_andThen(traceback, successHandler, failureHandler)
 	self._unhandledRejection = false
 
+	-- If we are already cancelled, we return a cancelled Promise
+	if self._status == Promise.Status.Cancelled then
+		local promise = Promise.new(function() end)
+		promise:cancel()
+
+		return promise
+	end
+
 	-- Create a new promise to follow this part of the chain
 	return Promise._new(traceback, function(resolve, reject, onCancel)
 		-- Our default callbacks just pass values onto the next promise.
@@ -1239,14 +1247,6 @@ function Promise.prototype:_andThen(traceback, successHandler, failureHandler)
 		elseif self._status == Promise.Status.Rejected then
 			-- This promise died a terrible death! Trigger failure immediately.
 			failureCallback(unpack(self._values, 1, self._valuesLength))
-		elseif self._status == Promise.Status.Cancelled then
-			-- We don't want to call the success handler or the failure handler,
-			-- we just reject this promise outright.
-			reject(Error.new({
-				error = "Promise is cancelled",
-				kind = Error.Kind.AlreadyCancelled,
-				context = "Promise created at\n\n" .. traceback,
-			}))
 		end
 	end, self)
 end
@@ -1430,28 +1430,37 @@ end
 
 --[[
 	Used to set a handler for when the promise resolves, rejects, or is
-	cancelled. Returns a new promise chained from this promise.
+	cancelled.
 ]]
-function Promise.prototype:_finally(traceback, finallyHandler, onlyOk)
+function Promise.prototype:_finally(traceback, finallyHandler)
-	if not onlyOk then
 	self._unhandledRejection = false
-	end
 
-	-- Return a promise chained off of this promise
+	local promise = Promise._new(traceback, function(resolve, reject, onCancel)
-	return Promise._new(traceback, function(resolve, reject)
+		onCancel(function()
+			-- The finally Promise is not a proper consumer of self. We don't care about the resolved value.
+			-- All we care about is running at the end. Therefore, if self has no other consumers, it's safe to
+			-- cancel. We don't need to hold out cancelling just because there's a finally handler.
+			self:_consumerCancelled(self)
+		end)
+
 		local finallyCallback = resolve
 		if finallyHandler then
-			finallyCallback = createAdvancer(traceback, finallyHandler, resolve, reject)
-		end
-
-		if onlyOk then
-			local callback = finallyCallback
 			finallyCallback = function(...)
-				if self._status == Promise.Status.Rejected then
+				local callbackReturn = finallyHandler(...)
-					return resolve(self)
-				end
 
-				return callback(...)
+				if Promise.is(callbackReturn) then
+					callbackReturn
+						:finally(function(status)
+							if status ~= Promise.Status.Rejected then
+								resolve(self)
+							end
+						end)
+						:catch(function(...)
+							reject(...)
+						end)
+				else
+					resolve(self)
+				end
 			end
 		end
 
@@ -1462,7 +1471,9 @@ function Promise.prototype:_finally(traceback, finallyHandler, onlyOk)
 			-- The promise already settled or was cancelled, run the callback now.
 			finallyCallback(self._status)
 		end
-	end, self)
+	end)
+
+	return promise
 end
 
 --[=[
@@ -1543,69 +1554,6 @@ function Promise.prototype:finallyReturn(...)
 	end)
 end
 
---[=[
-	Set a handler that will be called only if the Promise resolves or is cancelled. This method is similar to `finally`, except it doesn't catch rejections.
-
-	:::caution
-	`done` should be reserved specifically when you want to perform some operation after the Promise is finished (like `finally`), but you don't want to consume rejections (like in <a href="/roblox-lua-promise/lib/Examples.html#cancellable-animation-sequence">this example</a>). You should use `andThen` instead if you only care about the Resolved case.
-	:::
-
-	:::warning
-	Like `finally`, if the Promise is cancelled, any Promises chained off of it with `andThen` won't run. Only Promises chained with `done` and `finally` will run in the case of cancellation.
-	:::
-
-	Returns a new promise chained from this promise.
-
-	@param doneHandler (status: Status) -> ...any
-	@return Promise<...any>
-]=]
-function Promise.prototype:done(doneHandler)
-	assert(doneHandler == nil or isCallable(doneHandler), string.format(ERROR_NON_FUNCTION, "Promise:done"))
-	return self:_finally(debug.traceback(nil, 2), doneHandler, true)
-end
-
---[=[
-	Same as `andThenCall`, except for `done`.
-
-	Attaches a `done` handler to this Promise that calls the given callback with the predefined arguments.
-
-	@param callback (...: any) -> any
-	@param ...? any -- Additional arguments which will be passed to `callback`
-	@return Promise
-]=]
-function Promise.prototype:doneCall(callback, ...)
-	assert(isCallable(callback), string.format(ERROR_NON_FUNCTION, "Promise:doneCall"))
-	local length, values = pack(...)
-	return self:_finally(debug.traceback(nil, 2), function()
-		return callback(unpack(values, 1, length))
-	end, true)
-end
-
---[=[
-	Attaches a `done` handler to this Promise that discards the resolved value and returns the given value from it.
-
-	```lua
-		promise:doneReturn("some", "values")
-	```
-
-	This is sugar for
-
-	```lua
-		promise:done(function()
-			return "some", "values"
-		end)
-	```
-
-	@param ... any -- Values to return from the function
-	@return Promise
-]=]
-function Promise.prototype:doneReturn(...)
-	local length, values = pack(...)
-	return self:_finally(debug.traceback(nil, 2), function()
-		return unpack(values, 1, length)
-	end, true)
-end
-
 --[=[
 	Yields the current thread until the given Promise completes. Returns the Promise's status, followed by the values that the promise resolved or rejected with.
 

lib/init.spec.lua

@@ -556,9 +556,11 @@ return function()
 				count += 1
 			end)
 
-			root:andThen(function()
+			root
+				:andThen(function()
 					count += 1
-			end):cancel()
+				end)
+				:cancel()
 
 			resolve("foo")
 
@@ -636,7 +638,7 @@ return function()
 		it("should track consumers", function()
 			local pending = Promise.new(function() end)
 			local p0 = Promise.resolve()
-			local p1 = p0:finally(function()
+			local p1 = p0:andThen(function()
 				return pending
 			end)
 			local p2 = Promise.new(function(resolve)
@@ -703,18 +705,10 @@ return function()
 			expect(callCount).to.equal(5)
 		end)
 
-		it("should be a child of the parent Promise", function()
+		it("should not forward return values", function()
-			local p1 = Promise.new(function() end)
-			local p2 = p1:finally(function() end)
-
-			expect(p2._parent).to.equal(p1)
-			expect(p1._consumers[p2]).to.equal(true)
-		end)
-
-		it("should forward return values", function()
 			local value
 
-			Promise.resolve()
+			Promise.resolve(2)
 				:finally(function()
 					return 1
 				end)
@@ -722,7 +716,124 @@ return function()
 					value = v
 				end)
 
-			expect(value).to.equal(1)
+			expect(value).to.equal(2)
+		end)
+
+		it("should not consume rejections", function()
+			local catchRan = false
+			local thenRan = false
+			Promise.reject(5)
+				:finally(function()
+					return 42
+				end)
+				:andThen(function()
+					thenRan = true
+				end)
+				:catch(function(value)
+					catchRan = true
+					expect(value).to.equal(5)
+				end)
+
+			expect(catchRan).to.equal(true)
+			expect(thenRan).to.equal(false)
+		end)
+
+		it("should wait for returned promises", function()
+			local resolve
+			local promise = Promise.reject("foo"):finally(function()
+				return Promise.new(function(r)
+					resolve = r
+				end)
+			end)
+
+			expect(promise:getStatus()).to.equal(Promise.Status.Started)
+
+			resolve()
+
+			expect(promise:getStatus()).to.equal(Promise.Status.Rejected)
+			local _, value = promise:_unwrap()
+			expect(value).to.equal("foo")
+		end)
+
+		it("should reject with a returned rejected promise's value", function()
+			local reject
+			local promise = Promise.reject("foo"):finally(function()
+				return Promise.new(function(_, r)
+					reject = r
+				end)
+			end)
+
+			expect(promise:getStatus()).to.equal(Promise.Status.Started)
+
+			reject("bar")
+
+			expect(promise:getStatus()).to.equal(Promise.Status.Rejected)
+			local _, value = promise:_unwrap()
+			expect(value).to.equal("bar")
+		end)
+
+		it("should reject when handler errors", function()
+			local errorValue = {}
+			local promise = Promise.reject("bar"):finally(function()
+				error(errorValue)
+			end)
+
+			local ok, value = promise:_unwrap()
+
+			expect(ok).to.equal(false)
+			expect(value).to.equal(errorValue)
+		end)
+
+		it("should not prevent cancellation", function()
+			local promise = Promise.new(function() end)
+
+			local finallyRan = false
+			promise:finally(function()
+				finallyRan = true
+			end)
+
+			local consumer = promise:andThen(function() end)
+
+			consumer:cancel()
+
+			expect(promise:getStatus()).to.equal(Promise.Status.Cancelled)
+			expect(finallyRan).to.equal(true)
+		end)
+
+		it("should propagate cancellation downwards", function()
+			local finallyRan = false
+			local andThenRan = false
+			local root = Promise.new(function() end)
+
+			local consumer = root:finally(function()
+				finallyRan = true
+			end)
+
+			root:cancel()
+
+			expect(root:getStatus()).to.equal(Promise.Status.Cancelled)
+			expect(consumer:getStatus()).to.equal(Promise.Status.Cancelled)
+
+			expect(finallyRan).to.equal(true)
+			expect(andThenRan).to.equal(false)
+		end)
+
+		it("should propagate cancellation upwards", function()
+			local finallyRan = false
+			local andThenRan = false
+			local root = Promise.new(function() end)
+
+			local consumer = root:finally(function()
+				finallyRan = true
+			end)
+
+			consumer:cancel()
+
+			expect(root:getStatus()).to.equal(Promise.Status.Cancelled)
+			expect(consumer:getStatus()).to.equal(Promise.Status.Cancelled)
+
+			expect(finallyRan).to.equal(true)
+			expect(andThenRan).to.equal(false)
 		end)
 	end)
 
@@ -1168,20 +1279,6 @@ return function()
 		end)
 	end)
 
-	describe("Promise:doneReturn", function()
-		it("should return the given values", function()
-			local value1, value2
-
-			Promise.resolve():doneReturn(1, 2):andThen(function(one, two)
-				value1 = one
-				value2 = two
-			end)
-
-			expect(value1).to.equal(1)
-			expect(value2).to.equal(2)
-		end)
-	end)
-
 	describe("Promise:andThenCall", function()
 		it("should call the given function with arguments", function()
 			local value1, value2
@@ -1195,47 +1292,6 @@ return function()
 		end)
 	end)
 
-	describe("Promise:doneCall", function()
-		it("should call the given function with arguments", function()
-			local value1, value2
-			Promise.resolve():doneCall(function(a, b)
-				value1 = a
-				value2 = b
-			end, 3, 4)
-
-			expect(value1).to.equal(3)
-			expect(value2).to.equal(4)
-		end)
-	end)
-
-	describe("Promise:done", function()
-		it("should trigger on resolve or cancel", function()
-			local promise = Promise.new(function() end)
-			local value
-
-			local p = promise:done(function()
-				value = true
-			end)
-
-			expect(value).to.never.be.ok()
-			promise:cancel()
-			expect(p:getStatus()).to.equal(Promise.Status.Cancelled)
-			expect(value).to.equal(true)
-
-			local never, always
-			Promise.reject()
-				:done(function()
-					never = true
-				end)
-				:finally(function()
-					always = true
-				end)
-
-			expect(never).to.never.be.ok()
-			expect(always).to.be.ok()
-		end)
-	end)
-
 	describe("Promise.some", function()
 		it("should resolve once the goal is reached", function()
 			local p = Promise.some({