red-blox/Red

Implementing SharedEvents

Closed this issue · 7 comments

Figured I could take a shot at implementing these so you no longer have to explicitly call :Server and :Client when requiring events.

I've read the code you wrote for this around a month ago but since your mind may have changed on how to implement it in that time, I think it would be good to get it figured out here.

Call Modes

Call

Only one listener. It is called directly in a new thread, rather than going through the slight overhead of a Signal object.

Signal

Any number of listeners. Slightly worse for performance as it needs to go through the Signal:Fire code, which while very small, does add some slight overhead.

Event Declaration

Follows the same format as regular events

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Red = require(ReplicatedStorage.Packages.Red)

return Red.SharedEvent({
    Name = "Name",
    Unreliable = false,
    CallMode = "Signal"
}, function() end)

If the Reliability and CallMode, are not specified, it will choose sensible defaults.

local ReplicatedStorage = game:GetService("ReplicatedStorage")
local Red = require(ReplicatedStorage.Packages.Red)

-- Defaults to Reliable, and Signal call mode.
return Red.SharedEvent("Identifier", function() end)

Usage

Call Mode

-- Client
local SharedEvent = require(ReplicatedStorage.SharedEvent)
SharedEvent:SetClientListener(...)
SharedEvent:FireServer(...)

SharedEvent:FireClient(...) -- errors

-- Server
local SharedEvent = require(ReplicatedStorage.SharedEvent)
SharedEvent:SetServerListener(...)
SharedEvent:FireClient(...)

SharedEvent:FireServer(...) -- errors

Signal Mode

-- Client
local SharedEvent = require(ReplicatedStorage.SharedEvent)
SharedEvent:OnClient(...)
SharedEvent:FireServer(...)

SharedEvent:FireClient(...) -- errors

-- Server
local SharedEvent = require(ReplicatedStorage.SharedEvent)
SharedEvent:OnServer(...)
SharedEvent:FireClient(...)

SharedEvent:FireServer(...) -- errors

Final Note

I'm happy to write the tests for these once the implementation is finalized, but what were the issues in implementation you encountered when writing them? Would be useful to know so I don't end up running into the same brick wall.

Is there any reason that call events cannot yield? Also apologies, but I don't remember the issues I had, I think they just failed in tests and I didn't want to bother implementing them.

Is there any reason that call events cannot yield? Also apologies, but I don't remember the issues I had, I think they just failed in tests and I didn't want to bother implementing them.

I haven't thoroughly read the backend yet but I assumed since they aren't spawned, a yield would also delay all the other callbacks from the same batch of events.

Is there any reason that call events cannot yield? Also apologies, but I don't remember the issues I had, I think they just failed in tests and I didn't want to bother implementing them.

I haven't thoroughly read the backend yet but I assumed since they aren't spawned, a yield would also delay all the other callbacks from the same batch of events.

Nevermind, re-read it and this is not true. I'll amend the issue.

Here's a snag:
image
image
image

Since the function can return two different types depending on what arguments are passed, the intellisense doesn't know what to display. It also doesn't add placeholder parameter names when autocompleting.
This isn't a huge deal but does worsen the DX a little bit.

Here's the type in question:

type ConstructorType =
	(<T...>(Options: SharedCallEventOptions, Validate: ValidateFunction<T...>) -> SharedCallEvent<T...>)
	& (<T...>(Options: string | SharedSignalEventOptions, Validate: ValidateFunction<T...>) -> SharedSignalEvent<T...>)

I've got three solutions in mind:

  • Leave it as it is, it's a pretty minor issue to change the API over.
  • Create two different functions, one for creating a Signal event and one for creating a Call event. Since they have different APIs I think two different functions for creating them can make sense.
  • Unify the Call and Signal event types, so they both share the same API. This does lose the distinction between them outside of their declaration though, so when reading code you'll have to go back to the event creation to check if it's a Call or a Signal event.

I'm leaning towards the 2nd option, but it is very minor so just leaving it as is would probably be fine.

second option sgtm

I'm thinking of exposing it as SharedEvent (which would use Call mode) and SharedSignalEvent, since Call mode makes more sense as a default to me - it matches the behaviour of the previous Events API more closely, and more often than not you won't need more than one connection.
Writing out SharedCallEvent every time seems unnecessarily wordy to me.

Adding FireWithFilter from the old Event API too as FireFilteredClients