Sleitnick / Knit

Lightweight game framework for Roblox
https://sleitnick.github.io/Knit/
MIT License
566 stars 93 forks source link

Allow us to get access to uninitialized services / controllers #210

Closed Weldify closed 1 year ago

Weldify commented 2 years ago

Currently, to get access to a service/controller, you have to use .GetService() and .GetController() respectively in the :Start() method of another controller. It looks something like this:

local MyController = Knit.CreateController {
    Name = "MyController",
}

local OtherController

function MyController:KnitStart()
    OtherController = Knit.GetController("OtherController")
end

While this is fine, I don't personally like spending 2 lines to get access to each controller and I feel like there could be a better way. It could go something like this:

local MyController = Knit.CreateController {
    Name = "MyController",
}

local OtherController = Knit.GetController("OtherController", true)

function MyController:KnitStart()
    -- no need for anything in here
end

I suggest adding an optional argument to .GetService() and .GetController() that allows you to access a service/controller before it gets initialized.

Let me know what you think. I'd be happy to implement this and do a PR.

captalbator commented 2 years ago

This is not possible because controllers and services are only added to Knit when required, meaning that Knit has no knowledge of the controller or service existing beforehand.

Weldify commented 2 years ago

This is possible. And I, in fact, know how to implement it as well. The concept goes as such: When you use .GetService() with the optional argument, instead of erroring if the service isnt there, it will create a barebones dummy table including it's name and other required (although empty) fields like .Client for services. Then, when the service is actually created, all of the values from the service definition get copied into the existing table. That way, when you use .GetService() on a non-existing service, it will just supply you with a dummy table for a time being, and by the time :KnitStart() is called, it'll already turn into the actual service itself. Now, an error/warning would be great if by the time you start Knit itself the service wasnt created yet, but that's easily implementable.

ghost commented 2 years ago

.GetService() with a boolean argument just to let the method return you the initialized service is bad practice, learn more about boolean parameters here.

The concept it self I strongly don't support because it actually ruins the main point of Knit (to avoid nasty race conditions, even if one may not occur at a specific point). Additionally an additional method GetUninitializedService (or something similar to that) should rather be implemented, there should be clear distinction between GetService and the new method. Additionally, GetService("service", true) would immediately imply to the reader that the true argument is completely redundant here!

EncodedVenom commented 2 years ago

One potential solution to this is to have .GetService() calls before Knit.Start() return an empty table with metamethods that directly set the table after the start process is completed. That way, no unnecessary arguments or additional methods need to be used.

Issues such as non-existent services/controllers, indexing them before they are ready, etc, can be enforced using metamethods.

EncodedVenom commented 2 years ago

Out of curiosity, would incorporating this function that I just wrote up into Knit.GetService work?

local function getKnitService(service: string)
    return setmetatable({}, {
        __index = function(dummyTable)
            local success = Knit.OnStart():timeout(1/60):await() -- Hacky, but this is out of Knit's scope.
            if not success then
                error("Knit has not been initialized yet!")
            end
            local exists, serviceObjectOrWarning = pcall(function()
                return Knit.GetService(service)
            end)
            if not exists then
                setmetatable(dummyTable, {}) -- Don't create repetitive errors.
                error(serviceObjectOrWarning)
            end
            for k, v in pairs(serviceObjectOrWarning) do
                rawset(dummyTable, k, v)
            end
            setmetatable(dummyTable, {})
        end
    })
end
Weldify commented 2 years ago

Made a PR. https://github.com/Sleitnick/Knit/pull/211

Sleitnick commented 2 years ago

I think I'm a little uneasy with services existing in an unpopulated state. In other words, the returned service is not actually the service yet at all. The table ref returned is expected to be mutated (in an ideal world, services should have no mutations IMO, but Knit v2 will address that)

4x8Matrix commented 1 year ago

@Sleitnick as mentioned, your uneasy with the idea of unpopulated services ~ instead could we introduce a method to get premature services? (This would keep services - as services, but premature services as a way to define a reference to an not-yet-set service; Which would also avoid tampering with Knit services as a whole. )

I primarily want this change to help make the codebase cleaner with the definitions of services and controllers references completed at the top

local Knit = require(path.to.Knit)

local SessionService = Knit.GetPrematureService("SessionService")
Sleitnick commented 1 year ago

Services/controllers should not be accessed if they're uninitialized