Fix .AddObject type definition

[kineticwallet]

No description provided.

[OverHash]

✅

[OverHash]

Worth it to note that InstanceType<O> is equivalent to T.

[OverHash]

Should we be dropping ((this: O) => void) | ((_: O) => void) from here?

Though we should be using T, not O here if we keep it.

Might be more clear if we use ExtractKeys<T, () => void> rather than ExtractKeys<InstanceType<O>, () => void>

[OverHash]

Looking at Janitor's internals, we don't need ((this: T) => void) | ((_: T) => void>:

https://github.com/howmanysmall/Janitor/blob/af930bf8939b7758b23bf62fb651ab5f348cdc52/src/init.luau#L252

we can see that Janitor warns if the method doesn't exist on the created instance, and it will later do nothing. We should explicitly ban this behavior from our consumers.

[OverHash]

I'd like us to change these over to the explicit T type for clarity. Other than that, looks good 🚀

[kineticwallet]

InstanceType is needed!?

[kineticwallet]

There might be no difference between T and InstanceType, but some sources say its still needed in some cases.https://stackoverflow.com/questions/70364964/difference-in-typescript-between-types-instancetypetypeof-myclass-and-myc

[OverHash]

There might be no difference between T and InstanceType, but some sources say its still needed in some cases.https://stackoverflow.com/questions/70364964/difference-in-typescript-between-types-instancetypetypeof-myclass-and-myc

Sweet, my mistake. Let's use InstanceType then.

Also note that package-lock.json is needed to be updated via npm update.

[kineticwallet]

done

[OverHash]

What are your thoughts on this? It would remove the need for us to have a useless T and IO type:

	public AddObject<
		O extends Constructable<unknown>,
		M extends undefined | ExtractKeys<InstanceType<O>, () => void> | true,
		I extends keyof U | undefined = undefined,
	>(object: O, methodName?: M, index?: I, ...args: ConstructorParameters<O>): InstanceType<O>;

[kineticwallet]

Testing it

[kineticwallet]

I would add IO back so its less boilerplate.

[OverHash]

I would add IO back so its less boilerplate.

I'd prefer if we keep it out for now -- that way we have three types corresponding to the three parameters. Maybe in the future we add it.

[kineticwallet]

Having to define undefined is a bit annoying, but it works.

[OverHash]

Having to define undefined is a bit annoying, but it works.

It's required in the luau side, so yeah we have to have it:

local MyClass = {}
function MyClass.new(a, b, c)
	print(`Created with {a}, {b}, {c}`)
	return setmetatable({}, { __index = MyClass })
end

function MyClass:Destroy()
	print("Being destroyed")
end

local a = Janitor.new()
a:AddObject(MyClass, "Destroy", nil, 1, 2, 3)
a:Destroy()

[kineticwallet]

the newest version should work, LGTM

[kineticwallet]

IO stands for InstanceType of O

[OverHash]

IO stands for InstanceType of O

Can we make it just T? I'm happy to accept it as T.

[OverHash]

Thanks for your work ❤️