stravant / goodsignal

A Roblox Lua Signal implementation that has full API and behavioral parity with Roblox' RBXScriptSignal
MIT License
46 stars 12 forks source link

Difference in behaviour with BindableEvent:Destroy() when called during a connection handler #4

Open chess123mate opened 2 years ago

chess123mate commented 2 years ago
local x = Instance.new("BindableEvent")
for i = 1, 10 do
    local con; con = x.Event:Connect(function()
        x:Destroy()
        print("B", i)
    end)
end
x:Fire()

local x = require(script.GoodSignal).new()
for i = 1, 10 do
    local con; con = x:Connect(function()
        x:DisconnectAll()
        print("G", i)
    end)
end
x:Fire()

Only B 10 is printed for BindableEvent, but GoodSignal prints out G 10 through G 1. I point this out due to the claim

A Roblox Lua Signal implementation that has full API and behavioral parity with Roblox' RBXScriptSignal type.

This can be corrected by adding if not self._handlerListHead then break end after task.spawn(freeRunnerThread, item._fn, ...) in the Fire function.

lucasmz-dev commented 1 year ago

The better way to fix this is to have a proper public Connected field and set it to false on every connection when disconnecting. The fact that Connected isn't a thing in GoodSignal also shows that no, GoodSignal does not have full API & behavioral parity. (And no Deferred event support in a form of a separate module or one that automatically chooses depending on what mode the game is currently on, which isn't hard to come up with; mixing up signal behaviours (from native roblox events and dev-made ones) is stressful and causes hidden problems)

GoodSignal attempts at saving time by just setting the start node to nil, which overshadows the issue that he did notice with not checking Connected when firing when you disconnect connections in a certain way.