-
Notifications
You must be signed in to change notification settings - Fork 141
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Some minor suggestions.
src/assertDeepEqual.lua
Outdated
@@ -6,7 +6,7 @@ | |||
This should only be used in tests. | |||
]] | |||
|
|||
local function deepEqual(a, b) | |||
local function deepEqual(a, b): (boolean, string?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also work and more precisely express the scenarios:
local function deepEqual(a, b): (boolean, string?) | |
local function deepEqual(a, b): (true, nil) | (false, string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had no idea we could use boolean literals. Awesome!
@@ -19,7 +19,7 @@ local function deepEqual(a, b) | |||
visitedKeys[key] = true | |||
|
|||
local success, innerMessage = deepEqual(value, b[key]) | |||
if not success then | |||
if not success and innerMessage then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there's a narrowing bug lingering, using the above type suggestion should eliminate the needs for these extra checks. If it doesn't, add a -- FIXME Luau
comment and we can help file a Luau issue.
@@ -134,7 +134,7 @@ function BindingInternalApi.join(upstreamBindings) | |||
disconnect() | |||
end | |||
|
|||
disconnects = nil | |||
disconnects = nil :: any |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be better to make the delcaration of disconnects
have an explicit type that includes nil (eg local disconnects: Array<any> | nil = {}
). (Once deferred constraint resolution and some other Luau features land, it will not longer greedily assume the first assignment it sees is the only type possibility for the whole closure.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got a few analysis errors left from enabling strict mode in createReconsiler. My Luau knowledge is run up against a wall here so I would appreciate somebody taking a pass to clean up these remaining issues |
This is a good start towards type strictness! Thanks for introducing the tooling as well. |
This sets up the repo with Luau analysis and fixes the initial type errors
Checklist before submitting:
CHANGELOG.md