Knit icon indicating copy to clipboard operation
Knit copied to clipboard

Allow us to get access to uninitialized services / controllers

Open Weldify opened this issue 2 years ago • 8 comments

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.

Weldify avatar May 18 '22 18:05 Weldify

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.

captalbator avatar May 18 '22 18:05 captalbator

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.

Weldify avatar May 18 '22 18:05 Weldify

.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!

ghost avatar May 20 '22 05:05 ghost

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 avatar May 20 '22 19:05 EncodedVenom

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

EncodedVenom avatar May 21 '22 23:05 EncodedVenom

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

Weldify avatar May 22 '22 12:05 Weldify

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)

Sleitnick avatar May 23 '22 16:05 Sleitnick

@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")

4x8Matrix avatar Oct 02 '22 12:10 4x8Matrix

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

Sleitnick avatar Apr 06 '23 13:04 Sleitnick