Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Luau types for actions and reducers #70

Merged
merged 11 commits into from
Jun 9, 2022

Conversation

jkelaty-rbx
Copy link
Contributor

@jkelaty-rbx jkelaty-rbx commented Jun 7, 2022

Adds Luau types for actions and reducers, directly inspired by upstream Redux types.

- name: code quality (stylua)
shell: bash
run: |
stylua -c src/
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We ought to be running Selene ahead of darklua as well. We may even consider splitting it out into a separate quality CI job that runs selene/stylua/whatever analysis we can support, and one that runs darklua/tests/coverage reporting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reached out to the Selene maintainer and they graciously cut a release pretty quickly (thank you Kampfkarren!!) that supports my usage of generic type packs, so both checks are now running ahead of darklua. Let me know if you want me to move around the CI jobs any more than I already have.

@coveralls
Copy link

coveralls commented Jun 7, 2022

Coverage Status

Coverage increased (+0.04%) to 99.222% when pulling 097998a on jkelaty-rbx:actions-reducers-luau-types into 574ba20 on Roblox:master.

@github-actions
Copy link

github-actions bot commented Jun 7, 2022

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.

@jkelaty-rbx
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@jkelaty-rbx jkelaty-rbx marked this pull request as ready for review June 7, 2022 21:51
src/combineReducers.lua Outdated Show resolved Hide resolved
src/combineReducers.lua Outdated Show resolved Hide resolved

local function combineReducers<State>(map): Reducer<State>
return (
function(state, action)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe something like this and eliminate the hard cast?

Suggested change
function(state, action)
function<S>(state: S?, action: AnyAction): S

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a few things, but I settled on the any cast. I think this needs better generic type constraint support on the Luau side, otherwise you end up with some "unable to convert to State" errors, even when using the pseudo-constraint trick you showed me.

src/types/actions.lua Show resolved Hide resolved
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, this looks good! We'll need to be careful around how we cut the next version, since it'll break for folks who are running it in non-luau environments like lua 5.1 ci jobs

@jkelaty-rbx jkelaty-rbx requested a review from ZoteTheMighty June 9, 2022 20:12
@ZoteTheMighty ZoteTheMighty merged commit 07f634e into Roblox:master Jun 9, 2022
@jkelaty-rbx jkelaty-rbx deleted the actions-reducers-luau-types branch June 9, 2022 20:22
ZoteTheMighty pushed a commit that referenced this pull request Jun 13, 2022
Follows up on the types added in: #70.

Adds Luau types for thunks and the store. This is partially inspired by upstream Redux types here and here, but modified to be as useful as possible given Luau's constraints.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants