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 createAction #35

Merged
merged 6 commits into from Dec 3, 2020
Merged

Add createAction #35

merged 6 commits into from Dec 3, 2020

Conversation

ZoteTheMighty
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty commented Sep 18, 2018

Closes #31.

Adds an implementation of the generic action creator... creator that's used commonly for Rodux projects within Roblox. Looking for feedback on a decent name for this.

Potentially compelling possibilities for names:
defineAction or defineActionCreator
makeActionCreator (https://redux.js.org/recipes/reducingboilerplate#generating-action-creators)

Checklist before submitting:

  • Finalize name of method
  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

@ZoteTheMighty
Copy link
Contributor Author

The name referenced in the ticket bothers me because its a misnomer. The added function is not an action creator: it returns an action creator. A more appropriate name would be createActionCreator but that's pretty awkward

@coveralls
Copy link

coveralls commented Sep 18, 2018

Coverage Status

Coverage increased (+0.05%) to 99.401% when pulling bf326eb on ZoteTheMighty:action into 9e1736a on Roblox:master.

Copy link
Contributor

@SlartibartfastFjords SlartibartfastFjords left a comment

Choose a reason for hiding this comment

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

Should add documentation for this feature in docs/api-reference.md before submitting.

EDIT: Oh, my mistake, you already have that listed in your checklist before submitting :/.

`createAction` provides a utility that makes action creation cleaner
and less error prone. Define your Rodux action like this:

return Action("MyAction", function(value)
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments need to be updated to reflect this API.

@ZoteTheMighty
Copy link
Contributor Author

Right, I'd like to address the naming before getting too far into writing docs. Docs might also just mean transplanting a bunch of the comments into docs and thinning them out some in the actual code, as they're not as useful there.

The `type` field will be added automatically. Additionally, the returned action
creator now has a 'name' property that can be checked by your reducer:

local MyAction = require(Reducers.MyAction)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the convention to have this be require(Actions.MyAction)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll have to update these, as well as the wording in the asserts.

@ZoteTheMighty
Copy link
Contributor Author

ZoteTheMighty commented Oct 10, 2018

Having thought this over, I don't know if this approach is ubiquitous enough to justify this helper. Is this implementation too specific to exist as a general helper like createReducer and combineReducers? I've updated the naming and the wording of the affiliated comments and I filled in some documentation. If we think this might be useful to someone we can add it in, but I'd be fine with tabling it for now.

@LPGhatguy
Copy link
Contributor

Based on a renewed request in #31, we should pursue landing this. After thinking about it more, too, over the last... year and a half, this seems reasonable to me.

src/makeActionCreator.lua Outdated Show resolved Hide resolved
return function() end
end)

expect(FooAction).to.throw()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(FooAction).to.throw()
expect(FooAction).to.throw("must return a table")

nvm this is using a 3yr old version of test ez

@ZoteTheMighty
Copy link
Contributor Author

Commenting here to allow actions to be executed (I believe that's how this works!)

@github-actions
Copy link

github-actions bot commented Dec 2, 2020

CLA Signature Action:

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you read and sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just by adding a comment to this pull request with this exact sentence:

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

By commenting with the above message you are agreeing to the terms of the CLA. Your account will be recorded as agreeing to our CLA so you don't need to sign it again for future contributions to our company's repositories.

0 out of 1 committers have signed the CLA.
@paul Doyle

GitHub can't find an account for Paul Doyle.
You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@ZoteTheMighty
Copy link
Contributor Author

CLA bot was confused by merges of existing master commits, so I rebuilt this on top of master to clarify.

@ZoteTheMighty ZoteTheMighty merged commit f060348 into Roblox:master Dec 3, 2020
@benbrimeyer benbrimeyer mentioned this pull request Jan 4, 2021
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.

Add Rodux.createAction
5 participants