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

Suggestion, possible issues and improvements #41

Closed
doxick opened this issue Oct 17, 2018 · 7 comments
Closed

Suggestion, possible issues and improvements #41

doxick opened this issue Oct 17, 2018 · 7 comments

Comments

@doxick
Copy link

doxick commented Oct 17, 2018

based on my feedback on your reddit post, i tried to split out my comment in Improvements, suggestions and (possible) Issues

Improvements

Allow actionCreators from createAction to shape their payload

The current implementation of createAction returns an actionCreator which takes 1 argument: the payload.
If you want the payload to support multiple parameters (userId, userName, for instance), you'll have to manually recreate the object every time you use the actionCreator:

const updateUser = createAction('USER_UPDATE')
...
dispatch(updateUser({ userId: 10, data: { firstName: 'martijn', lastName: 'hermans' } }))

Will result in:

 {
   type: 'USER_UPDATE',
   payload: {
     userId: 10,
     data: {
       firstName: 'martijn',
       lastName: 'hermans'
     }
   }

By giving createAction a second parameter, which reduces the arguments passed to the actionCreator into a payload, you generate actionCreators which allow for multiple arguments.

const updateUser = createAction('USER_UPDATE', (userId, data) => ({ userId, data }) )
...
dispatch(updateUser(10, { firstName: 'martijn', lastName: 'hermans' })))

This will result in the same action, yet it becomes more re-usable, predictable and testable.
The default behaviour, without a "payload reducer" stays the same as it currently is: The first argument becomes the payload.

A basic implementation of this could be:

const identity = payload => payload
export function createAction(type, payloadReducer = identity) {
  const action = (...args) => ({
    type,
    payload: payloadReducer(...args)
  })
  action.toString = () => `${type}`
  return action
}

A more advanced implementation can be found in redux-actions/createAction.js

Exposing the slice Name/Type through .toString()

Exposing the slice 'key' has multiple benefits:

state[slice] will return the part of the state the slice operates on
createActions(slice, ....) provides the same prefix as the slice has given to the actions

combineReducers({
  [userSlice]: userSlice.reducer
})

you can now change the key within createSlice, and your whole app will function exactly as it did.

In my current implementation, the reducer returned from createReducer has a type: createReducer(type, actionsMap, initialState).
This is basically what the current createSlice(...).reducer does, but with the added benefit of re-using it as a constant key for its "domain". (usersReducer.toString() === 'books', etc)

Suggestions

createActions(prefix, actionMap)

In it's current state, createSlice allows you to create prefixed actionCreators.
These actions are however tied to the slice-reducer.
To create custom actionCreators which take the same prefix, you have to manually do that with createAction('users/add')

Prefixing actions has multiple functions:

  • Easier to group actions, which prevents collisions: users/add doesn't interfere with books/add
  • Easier to debug: the "domain"/slice which the actions belong to is visible in the type
  • The addition of createActions(prefix, actionMap) allows you to create prefixed actions the same way createSlice does, without having them tied 1-1 to a reducer.
  • createActions can be re-used within createSlice, but exposes the same functionality without having to create a full slice

The return type of createActions can either be an

  • Object: the actionCreators mapped to the keys
  • Array: the actionCreators returned in order of the actionMap
    From doing +- 15 projects using createActions, the Array version is easier to destructure and export

Return-type: Object

export const {
  add: addUser,
  update,
  'do-something-else': doSomethingElse
} = createActions('users', {
  add: (data) => ({ data }),
  update: (userId, data) => ({ userId, data }),
  'do-something-else': (userId, firstName) => ({userId, data: { firstName }})
})
export const {
  update
} = createActions('books', {
  update: (bookId, data) => ({ bookId, data })
})
  • not every action type lends itself for a variable name
  • unless you export the complete returned Object, you'll probably rename every destructured variable. (add => addUser)
  • destructuring the object version leads to naming collisions if done in the same file (users.update vs book.update

Return-type: Array

export const [
  addUser,
  updateUser,
  doSomethingElse
] = createActions('users', {
  add: (data) => ({ data }),
  update: (userId, data) => ({ userId, data }),
  'do-something-else': (userId, firstName) => ({userId, data: { firstName }})
})
  • the resulting Array can't be exported without destructuring (unless you want to use actions by their index... not very likely)
  • saves boilerplate when destructuring compared to the Object-version

(Possible) issues

actionCreators from createAction have no validation/type safety for the payload

Since the payload is whatever the first argument is i put in the actionCreator, i can easily mess up the data i put in there.
Code-completion doesn't hint what needs to be added.
Testing is not possible, since the input is the literal output for the payload
I have to repeat the same object shape every time i dispatch the action. Not very DRY

Creating actions in slices can lead to collision

The 'type' of the slice optional.
If i make two slices, both with no type, and both with an add action, the type of both actionCreators is add, which will lead to naming collision.

// books-slice.js
export const { actions } = createSlice({
  reducers: {
    add: (state, {payload}) => [...state, payload]
  }
})

// users-slice.js
export const { actions } = createSlice({
  reducers: {
    add: (state, {payload}) => [...state, payload]
  }
})

// some-component.js
import {actions: UserActions} from './users-slice'
...
dispatch(UserActions.add(user))
// i now have added both a user and a ... book?

When testing either reducer, you will not run into any issues.
When testing the combined Reducer, you will.
You'll probably also not check your sliceReducer again since that just passed a test and works correctly.
The cause of the error will be very hard to find since the implementation is abstracted away.

Create actions which aren't handled by a reducer for a slice

Since a slice roughly gets treated as a "domain", it would make sense to be able to define actions in createSlice which don't have a direct connection with a reducer, for instance to catch that action in a middleware.
_Theoretically, you can do that by just adding them with a state => state reducer
You do want to have actions within the same domain to have the same prefix, since that just makes it easier to debug (and more consistent)

I can't add my custom actions to be reduced by a slice-reducer

Theoretically, i can, by wrapping the slice reducer in another reducer, etc.
Not every action will be created within a slice reducer, yet i will possibly want those handled by the slice reducer. Currently, this is not possible.

Not being able to add custom actions to a slice Reducer or creating non-handled actions in a sliceReducer means i will have to mix different mindsets.
I will expose actions from a slice, but also have to create them separately somewhere else. This results in having to import them from 2 locations, which means actions are now coupled with slices.
I will create reducers with Slices but if i want to handle actions from userSlice in bookSlice, i can no longer create bookSlice but have to write the reducer manually again. There is no way to let bookSlice have a reducer which handles actions from userSlice.

I hope this provides a bit more structured version of my Reddit comment.

@markerikson
Copy link
Collaborator

@doxick : Thanks for the feedback! As I said on Reddit, it's excellent - detailed thoughts, and good ideas.

I definitely agree that making the payload be just "whatever got passed in to the action creator" is not ideal, even if this is supposed to be sort of the "opinionated / 80% use case" lib.

If you look at the documentation for https://github.com/ericelliott/autodux , they've got some of this stuff in there already (in particular, optionally providing functions to construct the payload). I think https://github.com/redux-utilities/redux-actions can do the same thing.

I'm totally in favor of adding that to our version of createSlice() as well. @neurosnap did the original implementation and didn't happen to include that, which is fine - I think we were mostly going for an initial MVP anyway.

I think I generally agree with the other concerns you've listed, but for some reason my brain isn't entirely wanting to work through them at the moment. If you've got further suggestions, please go ahead and comment with them, or file some PRs to start adding these pieces.

@neurosnap
Copy link

neurosnap commented Oct 20, 2018

Allow actionCreators from createAction to shape their payload

I like this idea. createAction was primarily created out of how I use actions, so I can see how some customization might be helpful since it isn't only used by createSlice.

Exposing the slice Name/Type through .toString()

I like this idea. We could also simply add a slice property to the returned object. The way I've been getting around this is by creating a const before sending it to createSlice, but I could see the value in wanting it returned.

createActions(prefix, actionMap)

I have no comment on this one, I've never needed this many helper functions for a project but that doesn't mean it isn't useful.

actionCreators from createAction have no validation/type safety for the payload

robodux leverages typescript for type safety. It's not perfect but it has gotten me most of what I need in terms of type safety. The plan is to bring all those types to this project once typescript is officially supported.

Creating actions in slices can lead to collision

This is certainly a possibility but since createSlice do not interact with others there is not much we can do in this regard.

What we could do is instead of passing the slice.reducer to combineReducers we could pass all of the slices into a combineSlices function that then checks for collisions and returns the combined reducer. Happy to entertain other thoughts.

This is a library I wrote awhile back that does this but at the package or feature level: https://github.com/neurosnap/redux-package-loader

Create actions which aren't handled by a reducer for a slice

Since my "domains" are actually at the package or feature level, not the slice level, this is personally a non-issue for me. Each package contains zero-to-many slices as well as side-effects, custom actions, custom selectors, etc.

Couldn't one do something like this?

const todos = createSlice({ ... });
todos.actions.more = createAction(`${todos.slice}/more`);

I can't add my custom actions to be reduced by a slice-reducer

I've never run into a situation where I needed this ... but I've only been using createSlice for a few projects so far. How would we approach accomplishing this?

@doxick
Copy link
Author

doxick commented Oct 22, 2018

Allow actionCreators from creatAction to shape their payload

I've created a pull request for this feature.

Exposing the slice Name/Type through .toString()

I've created a pull request for this feature.
Currently the createSlice function returns an object with a slice attribute, this just exposes the functionality to the reducer might you not want to use the createSlice function.

createActions(prefix, actionMap)

On the projects i've done for which we use redux, we generally run into 50-100+ actions in total, spread over multiple files. It reduces the amount of boilerplate while creating a very consistent way to provide actions.
This is merely a convenience function over having to do multiple createActions and adding the prefix yourself.

This is basically what createSlice already does (map an actionMap to actionCreators), yet createSlice doesn't allow you to provide a payloadCreator since you provide the reducer function. autoduxs createSlice allows you to provide both (Function or Object with create and reducer keys)

While the function itself is quite easy, i'm not sure wether to return an Array or an Object with the actions. I can create a PR for both and we could discuss it in the PR

actionCreators from createAction have no validation/type safety for the payload

Being able to shape the payload with a payloadCreator already provides a certain way to use the action without having to know the shape of the payload itself.
Accepting the PR for createAction with a payloadCreator would already cover this.

Creating actions in slices can lead to collision

I like the idea of combining slices, since it's more or less an extension of the already existing combineReducers.
I'm not sure how something like that would like. It would possibly be a very limited function which has little to no value outside of this library.
Any thoughts on making the slice parameter mandatory?

@markerikson
Copy link
Collaborator

@doxick : thanks for the PRs! I'll be busy all week between work and ReactConf, but I'll try to take a look at where things stand sometime next week.

@icopp
Copy link

icopp commented Nov 30, 2018

I think one thing that would be worth also thinking about here would be ways to have one action span slices. This is easily done with separate createAction and createReducer usage, but that makes it impossible to have same-or-similar syntax for everything if you use createSlice for simple boilerplate cases.

A basic example:

import { combineReducers, createAction, createReducer } from 'redux-starter-kit';

export const setValueA = createAction('SET_VALUE_A');
export const setValueB = createAction('SET_VALUE_B');
export const setValueAB = createAction('SET_VALUE_AB');

export const reducer = combineReducers({
    valueA: createReducer(null, {
        [setValueA]: (state, action) => action.payload,
        [setValueAB]: (state, action) => action.payload.a
    }),

    valueB: createReducer(null, {
        [setValueB]: (state, action) => action.payload,
        [setValueAB]: (state, action) => action.payload.b
    })
});

// dispatch(setValueA(1))
// dispatch(setValueB(2))
// dispatch(setValueAB({ a: 1, b: 2 }))

At the moment, to use this approach with createSlice, you would have to do something like this:

import reduceReducers from 'reduce-reducers';
import { combineReducers, createAction, createReducer } from 'redux-starter-kit';

const valueA = createSlice({
    slice: 'valueA',
    initialState: null,
    reducers: {
        setValueA: (state, action) => action.payload
    }
});

export const valueB = createSlice({
    slice: 'valueB',
    initialState: null,
    reducers: {
        setValueB: (state, action) => action.payload
    }
});

export const setValueAB = createAction('SET_VALUE_AB');

export const reducer = combineReducers({
    valueA: reduceReducers(
        createReducer(null, {
            [setValueAB]: (state, action) => action.payload.a
        }),
        valueA.reducer,
    ),

    valueB: reduceReducers(
        createReducer(null, {
            [setValueAB]: (state, action) => action.payload.b
        }),
        valueB.reducer,
    ),
});

// dispatch(valueA.actions.setValueA(1))
// dispatch(valueB.actions.setValueB(2))
// dispatch(setValueAB({ a: 1, b: 2 }))

A use case of this in the wild would be an SPA that caches information across multiple slices when using some network calls, while only dispatching per-slice actions on other network calls or with user actions.

@nemonweb
Copy link

@markerikson Hi. Awesome work. Thanks. What do you think about add redux-batched-actions ?

@markerikson
Copy link
Collaborator

Swinging back around to look at this. I think we've actually covered almost all the requests here at this point:

  • Allow action creators from createAction to shape their payloads, action creators have no validation for the payload: fixed thanks to "prepare callbacks" in createAction
  • Expose slice name: available as sliceObj.name
  • createActions with common prefix: not currently covered, but I don't see a strong need to add this right now
  • creating actions can have collisions due to lack of name: the name field will be required in 0.8
  • can't add custom actions to be reduced: handled by extraReducers

Based on that, I'm going to go ahead and close this. Thanks for the feedback, and hopefully the changes prove useful!

phryneas added a commit that referenced this issue Nov 2, 2021
* fix(generate): if a baseQuery was specified it should not called
* test(cli): --baseQuery option assign baseQuery
* test(cli): fetchBaseQuery called with the url provided by --baseUrl
* fix(generate): generate baseQueryCall if needed
* fix(generate): rename baseQueryCall to fetchBaseQueryCall
* fix(generate): pass fetchBaseQueryCall as value
* feat(generate): add a warning when --baseUrl is used along with --baseQuery

Co-authored-by: Lenz Weber <mail@lenzw.de>
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

No branches or pull requests

5 participants