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

redux-resource-action-creators #250

Merged
merged 2 commits into from
Dec 10, 2017
Merged

redux-resource-action-creators #250

merged 2 commits into from
Dec 10, 2017

Conversation

whichsteveyp
Copy link
Contributor

Implements the discussion and API in #186 that can be leveraged in both plugins & applications using redux-resource to help create valid action shapes for our ecosystem.

Published: [email protected] so we can try it out in apps and take a look at how this API works in practice after the bikeshedding that produced it ;)

@whichsteveyp whichsteveyp added the ecosystem extra This relates to something in the Redux Resource ecosystem, and not the core library label Nov 6, 2017
@whichsteveyp whichsteveyp self-assigned this Nov 6, 2017
Copy link
Owner

@jamesplease jamesplease left a comment

Choose a reason for hiding this comment

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

I'll do a more thorough review soon; just did a quick pass at some of the build-related stuff. Thanks for putting this together @sprjr – looks like a great start!

"types",
"type",
"checking"
],
Copy link
Owner

Choose a reason for hiding this comment

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

some of these should be removed since they refer to prop types

"type",
"checking"
],
"author": "James Smith <[email protected]>",
Copy link
Owner

Choose a reason for hiding this comment

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

this should prob be you @sprjr ✌️

var env = process.env.NODE_ENV;
var config = {
format: 'umd',
moduleName: 'ReduxResourcePropTypes',
Copy link
Owner

Choose a reason for hiding this comment

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

ReduxResourceActionCreators

var config = {
format: 'umd',
moduleName: 'ReduxResourcePropTypes',
external: ['redux-resource', 'prop-types'],
Copy link
Owner

Choose a reason for hiding this comment

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

'prop-types' isn't needed here

moduleName: 'ReduxResourcePropTypes',
external: ['redux-resource', 'prop-types'],
globals: {
'prop-types': 'PropTypes',
Copy link
Owner

Choose a reason for hiding this comment

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

this line can be removed

expect(createActionCreators).to.be.a('function');
});

describe(' - Creating Action Creators', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Let's drop the " – " prefix for these nested describes

@whichsteveyp
Copy link
Contributor Author

whichsteveyp commented Nov 8, 2017

Was chatting today about the current API here with @jmeas. Here is what we do right now:

.pending(options)
.null(options)
.succeeded(resources, options)
.failed(error, options)

The last two methods have a different API right now, although they are for different reasons.

  1. .succeeded accepting a resources array instead of { resources } on the options helps us enforce that you provide an array of resources for a success and that we can properly include that on the base of the action. This is something that our reducers will require so it's 'critical' for this action shape.
  2. .failed accepting an error was doing this to help enforce that there is an error present, but this was really only because redux-resource-xhr uses this key, and as it turns out there's nothing in there that is directly used by our reducers.

We talked about changing the APIs to one of these, although we're open to more suggestions:

.pending(resources, options)
.null(resources, options)
.succeeded(resources, options)
.failed(resources, options)

Which would retain the 'make sure you pass some resources for any of your actions' , although they are not necessarily required for these in all cases (e.g. you could just have a list or a label perhaps in the create scenarios.

Another suggestion was:

.pending({ resources, ...options, })
.null({ resources, ...options, })
.succeeded({ resources, ...options, })
.failed({ resources, ...options, })

Both of these would allow a consistent API across all 4 methods, although they have different tradeoffs or gains I would imagine. Any thoughts? /cc @vinodkl @marlonpp @tbranyen

This was referenced Dec 7, 2017
@jamesplease
Copy link
Owner

jamesplease commented Dec 8, 2017

@sprjr , I thought a bit about this while doing #275 / #277 . I'm currently thinking that the API should be:

// Create a set of action creators. `actionDefaults` will be set on every action.
var actionCreators = new createActionCreators('read', actionDefaults);

// Send off the `pending` action. Add/override `actionDefaults` with `action`.
actionCreators.pending(action);
actionCreators.failed(action);
actionCreators.succeeded(action);
actionCreators.null(action);

I think there are two benefits to using this API:

  • there's usually a lot of shared data with the start/end actions, and since there are four total actions, it is awesome to be able to specify that in one place, actionDefaults
  • using the actionTypes export directly sucks, and specifying read and then using .pending() is much nicer

My concern with the different signatures in the current implementation is that it may be too much for folks to remember.

whichsteveyp added a commit that referenced this pull request Dec 8, 2017
This update changes the action creator utilities to the discussed API signature in #250
in which each method simply calls '.pending(action)' to produce a redux-resource action.
jamesplease pushed a commit that referenced this pull request Dec 10, 2017
This update changes the action creator utilities to the discussed API signature in #250
in which each method simply calls '.pending(action)' to produce a redux-resource action.
@jamesplease jamesplease force-pushed the action-creators branch 3 times, most recently from 460f808 to 05b5c38 Compare December 10, 2017 01:07
@jamesplease jamesplease merged commit ddf5e9a into master Dec 10, 2017
@jamesplease jamesplease deleted the action-creators branch December 10, 2017 01:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ecosystem extra This relates to something in the Redux Resource ecosystem, and not the core library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants