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

initial work on create slice #37

Closed
wants to merge 3 commits into from
Closed

Conversation

@neurosnap
Copy link

neurosnap commented Aug 22, 2018

Extracted from this comment: #17 (comment)

#17

@nickmccurdy nickmccurdy added this to To do in Roadmap Aug 30, 2018
@nickmccurdy nickmccurdy moved this from To do to To review in Roadmap Aug 30, 2018
@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Aug 30, 2018

Thanks for the contribution.

This looks similar to createReducer but I like how the actions are included in the results. Do you think we could include this in the existing createReducer API or do you want it to be seperate?

As far as TypeScript support goes, I have State inference working in #12. I can't get Actions working yet but I'd appreciate a review if you have time.

@neurosnap

This comment has been minimized.

Copy link
Author

neurosnap commented Sep 6, 2018

I updated the PR with the requested changes. I also added some tests. Happy to make more changes as recommended.

@neurosnap neurosnap force-pushed the neurosnap:create-slice branch from 69c1c8e to d65dedb Sep 6, 2018
@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Sep 7, 2018

Thanks. So the goal compared to createReducer is to bundle in actions instead of just returning a merged slice reducers, right? Do you think we could make the API more like createReducer with initialState, actionsMap, and slice as ordered args? Also, why is the slice property in the returned bundled?

@neurosnap

This comment has been minimized.

Copy link
Author

neurosnap commented Sep 10, 2018

So to clarify you would like to see:

const counter = createSlice({
  initialState,
  actionsMap,
  slice,
});

I added slice as a way to get the slice back as a const if a string literal was used inside the createSlice function. I can remove that.

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Sep 10, 2018

Ah, so it's just for namespacing actions? That sounds like it could still be useful, and I do like that API signature more now.

@markerikson What do you think about this API for wrapping createReducer in reducer and action bundles with optional slice based namespacing?

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Sep 10, 2018

Uh... yes, that was exactly the point in the first place :) Like autodux, but using our own createReducer internally.

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Sep 10, 2018

Cool, I'm in favor of merging as long as we can get the order in #37 (comment)

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Sep 10, 2018

I think I'd prefer a single "options object" rather than multiple positional arguments for this API, but it's up for discussion.

@nickmccurdy

This comment has been minimized.

Copy link
Collaborator

nickmccurdy commented Sep 10, 2018

createReducer already has a signature of (initialState, actionsMap), so I'd prefer to stay consistent by either following the order in #37 (comment) or also converting createReducer to an options object signature.

Is there a use case where it would make sense for the user to not pass actions as an object? I noticed that it's optional for createSlice but not createReducer, which seems inconsistent to me. Can you remove = {}?

@neurosnap

This comment has been minimized.

Copy link
Author

neurosnap commented Sep 10, 2018

options object makes sense to me personally, but I don't care either way.

This library also has nothing to do with creating selectors like autodux does, so it is not at perfect parity. I personally think that auto-generating selectors will really bloat this code and beyond a root selector for each slice I wouldn't add anything more than that.

@neurosnap

This comment has been minimized.

Copy link
Author

neurosnap commented Sep 10, 2018

Also, it might be useful to write a little helper for creating actions that anyone can use. Example:

export default function createAction(type) {
  const action = (payload) => ({
    type,
    payload,
  });

  action.toString = () => `${type}`;
  return action;
}

// usage
const increment = createAction('INCREMENT');
console.log(increment);
// -> 'INCREMENT'

This will allow us to stringify the action returned from createSlice and is something very similar to what autodux does. This will allows createSlice to work with libraries like redux-saga

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Sep 10, 2018

I'd actually like to clone the "create selectors automatically" functionality if we could.

@neurosnap

This comment has been minimized.

Copy link
Author

neurosnap commented Sep 10, 2018

Ok, if it's alright with you I would prefer to add that in as a separate PR. It seems distinct enough to be its own feature.

@markerikson

This comment has been minimized.

Copy link
Collaborator

markerikson commented Oct 14, 2018

Superseded by the work done on robodux and ported directly to redux-starter-kit, as discussed in #17 . @neurosnap , thanks for writing robodux and allowing us to port that over!

Roadmap automation moved this from To review to Done Oct 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
  
Done
3 participants
You can’t perform that action at this time.