roact-rodux

roact-rodux copied to clipboard

This PR adds types for the dispatch prop. This includes a standard action dispatch and a thunkful action dispatch variant.

Inspired by: https://github.com/reduxjs/react-redux/blob/master/src/types.ts

This RP also adds typechecking to CI with luau-lsp.

Coverage decreased (-67.9%) to 32.09% when pulling 85fcd6690ac2ddb7f0c1cd859d38b53cd29cddd8 on jkelaty-rbx:action-thunk-prop-types into d7ef4d823a28503a13dcfe1f15c0f48b6dd6cee4 on Roblox:master.

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.

I think this mostly looks good, but I can see that one of your previous analysis "failures" still got marked as green: Can we introduce an analyze error in a commit, validate that CI fails, and the revert it before merge? I've had odd behaviors with tools exiting with an error and still passing CI steps, so I'm perhaps a bit paranoid 😅

Yeah, I'm guessing (likely mistakingly) luau-lsp doesn't factor a missing directory for its exit code. My test commit successfully failed CI, so we should be good!

@ZoteTheMighty Thanks for the quick review! Feel free to merge whenever

Looking good, but I'm curious: Are these types enough to catch bugs?

@matthargett This is a great question. These types might catch bugs where we try to pass something other than an action or a thunk into dispatch, but their main utility is unfortunately to work around some of Luau's constraints. The issue that I ran into was when I was dispatching non-homogenous types within the same dispatch scope. Luau would try to narrow the dispatch arg on the first invocation and then complain about subsequent calls, thus type checking fails.

However, I just did some quick testing to verify what I said, and I stumbled onto another issue with void returning thunks that I will need to follow up on, so we should put this PR on pause until then anyway.