Repository navigation
Resolve typechecking errors with strict Luau in Trove. - #263
MaximumADHD wants to merge 8 commits into
Conversation
|
Bless! |
|
Nice, good timing with #261. I'll check this out over the weekend |
There was a problem hiding this comment.
I haven't seen much moving this PR forward so I figured I'd give my two cents on this. Worth noting some of my comments are relevant to a couple places in the file, but I didn't make duplicates so it's worth doing a scan for other places the comment may be relevant as well.
I think most of these problems are fixable with the exception of the cleanupMethod arg issue I made note of. That functionality may need to be seperated into a seperate method in order for typing to be supported.
I found a number of these issues by looking at the test file and hover checking the typing. It would probably be beneficial to add more tests or modify the existing ones to make use of the type checking a bit more. This should help ensure everything is working correctly if future changes are made.
| type SignalLike<T... = ...any> = { | ||
| Connect: (self: any, callback: (T...) -> ()) -> ConnectionLike, | ||
| Once: (self: any, callback: (T...) -> ()) -> ConnectionLike, | ||
| } |
There was a problem hiding this comment.
In ConnectionLike self is a generic, but here it's typed as any. Similarily it's possible to also pass a generic in to the resulting ConnectionLike/
In my experience it's best to be consistent and given it's possible to support the generics here. It is a bit more verbose, but I don't see any downside in supporting it.
export type SignalLike<T = any, U = any, V... = ...any> = {
Connect: (self: T, callback: (V...) -> ()) -> ConnectionLike<U>,
Once: (self: T, callback: (V...) -> ()) -> ConnectionLike<U>,
}| type SignalLikeMetatable<T... = ...any> = typeof(setmetatable( | ||
| {}, | ||
| {} :: { | ||
| Connect: (self: SignalLikeMetatable, callback: (...any) -> ...any) -> ConnectionLike | ConnectionLikeMetatable, | ||
| Once: (self: SignalLikeMetatable, callback: (...any) -> ...any) -> ConnectionLike | ConnectionLikeMetatable, | ||
| Connect: (self: SignalLikeMetatable<T...>, callback: (T...) -> ()) -> ConnectionLike, | ||
| Once: (self: SignalLikeMetatable<T...>, callback: (T...) -> ()) -> ConnectionLike, | ||
| } | ||
| )) |
There was a problem hiding this comment.
Is there a reason this can't be written in a similar format to the ConnectionLikeMetatable?
export type SignalLikeMetatable<T = any, U = any, V... = ...any> = typeof(setmetatable({}, {} :: SignalLike<T, U, V...>))| getStatus: (self: T) -> PromiseLikeStatus, | ||
| finally: (self: T, callback: (...any) -> ...any) -> T, | ||
| cancel: (self: T) -> (), | ||
| } |
There was a problem hiding this comment.
There isn't promise typing right now in evaera's promise lib, but there are some forks that add a typing. Using a generic typepack for the callback may be beneficial for anyone using those packages.
Additionally, it's possible that the returned promise will not be of the same shape as the original promise so a different generic should be returned from finally.
export type PromiseLike<T = any, U = any, V... = ...any, W... = ...any> = {
getStatus: (self: T) -> PromiseLikeStatus,
finally: (self: T, callback: (V...) -> W...) -> U,
cancel: (self: T) -> (),
}| --- INTERNAL FUNCTION, DO NOT USE! | ||
| -- FIXME: Just make this a local function if it isn't used elsewhere? Is it used for testing? | ||
| function Trove._cleanupObject(_self: any, object: any, cleanupMethod: string?) |
There was a problem hiding this comment.
I agree with this comment. If this doesn't need to be exposed then it should be made into a local function.
| ``` | ||
| ]=] | ||
| function Trove.Add(self: TroveInternal, object: Trackable, cleanupMethod: string?): any | ||
| function Trove.Add<T>(self: Trove, object: Trackable<T>, cleanupMethod: string?): T |
There was a problem hiding this comment.
In order to support the proper output we need two generics:
function Trove.Add<T, U>(self: Trove, object: T & Trackable<U>, cleanupMethod: string?): TThis makes it so you can do stuff like local part = trove:Add(Instance.new("Part")) and actually get a part back.
This is true for many of the methods here btw, but I'm not commenting on all of them.
Seperately, I'm not sure this method can properly handle typing with the cleanupMethod unless you used the new solver + a type function.
| function Trove.Connect<T...>( | ||
| self: Trove, | ||
| signal: SignalLike<T...> | SignalLikeMetatable<T...> | RBXScriptSignal<T...>, | ||
| fn: (T...) -> () | ||
| ): ConnectionLike |
There was a problem hiding this comment.
Returning ConnectionLike here is not accurate enough imo.
If you pass an actual RbxScriptSignal you should get a RbxScriptConnection back.
This branch has not been deployed
4 participants
Summary
This commit extends on #200. The recent changes to support
--!strictin Trove were good in principle, but had several problems because of Luau being Luau. This pull request attempts to resolve a lot of these problems.Unlike before, this allows the actual metatable-backed Trove type to be exported!
What was changed?
I've removed the manually-defined
Trovetype in favor of renaming theTroveInternaltype toTrove. Function signatures are now derived as they are written in theTrovetable. I also updated many of the types inTrackableto be generic-backed (defaults set toanyand...anywhen possible), which allows forTto be slotted in as the value ofself. It just works™!Hard to say is this is 100% bug-free, but I've been thoroughly checking against my
--!strictcodebase and the results are extremely promising!