Defer disconnecting connections in signal:Destroy() to reflect native RBXScriptSignal behavior + Trove type improvements#249
Conversation
… RBXScriptSignal behavior + Defer disconnecting connections in signal:Destroy(), so that any calls to signal:FireDeferred(...) during the same resumption cycle but before the :Destroy() call will fire to the connections. This reflects native rbxScriptSignal:Fire() behavior and instance:Destroy() behavior with workspace.SignalBehavior set to Deferred. Move signal:DisconnectAll() behavior to new function called disconnectSignalConnectionsStartingFromItem and removed references to _handlerListHead in the new function. _handlerListHead is cleared and sent to the new function from signal:DisconnectAll() and signal:Destroy() instead. Changed some comments which may need to be rewritten or new ones may need to be added.
|
I added some type improvements to Trove to this pull request. I would make a new pull request for this, but I do not see a simple way of doing that, so this will do. Improvements are as follows:
local cleanup = Trove.new()
local signal = Signal.new()
cleanup:Connect(signal)
local cleanup = Trove.new()
local event = TypedRemote.event("foo") :: TypedRemote.Event<>
cleanup:Connect(event.OnServerEvent, function() end)The union type errors are issues I ran into related to the native Roblox type checker. |
signal:Destroy() to reflect native RBXScriptSignal behaviorsignal:Destroy() to reflect native RBXScriptSignal behavior + Trove type improvements
modules/signal/init.luau
Defer disconnecting connections in
signal:Destroy(), so that any calls tosignal:FireDeferred(...)during the same resumption cycle but before thesignal:Destroy()call will fire to the connections._proxyHandlerdisconnection is deferred with the rest of the connections.This reflects native Roblox behavior when calling
rbxScriptSignal:Fire(...)and then destroying a parentInstance, which still fires to its connections even withworkspace.SignalBehaviorset toDeferred.These changes bring back old behavior as outlined in Cancel waiting threads if signal disconnects all connections #164 (comment), but only when
signal:Destroy()is used instead.signal:DisconnectAll()retains its current behavior.Move signal:DisconnectAll() behavior to new function called
disconnectSignalConnectionsStartingFromItemand removed references to_handlerListHeadin the new function._handlerListHeadis cleared immediately and sent to the new function from bothsignal:DisconnectAll()andsignal:Destroy()instead.connection:Disconnect()is unchanged as callingrbxScriptSignal:Fire(...)and thenrbxScriptConnection:Disconnect()does not fire to therbxScriptConnectionwhen usingDeferredSignalBehavior.connection:Destroy()is unchanged as there is no equivalent to it inRBXScriptConnection.Changed some comments and function documentation which may need to be rewritten and new ones may need to be added. Documentation for
signal:Destroy()should be updated at the very least.modules/signal/init.test.luau
Added 3 new tests
Added new function called
DeferConditionto replaceAwaitConditionwheresignal:FireDeferred(...)is used for better test accuracy in theory.Tests were not ran in the other modules so it's possible behavior could be different, although unlikely.