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

Switches to a domain-sliced approach to our Redux setup. #356

Merged
merged 9 commits into from Sep 9, 2016

Conversation

5 participants
@skellock
Contributor

skellock commented Sep 6, 2016

This is inspired by @mmazzarolo and others in [https://github.com//issues/158]

It's kinda like https://github.com/erikras/ducks-modular-redux in spirit, but the implementation is slightly more forgiving.

For the most part this has been pretty positive! There's a few things I'm concerned about, but that's getting nitpicky. Rather than say what they are, I'll let you all come to your own conclusions! :)

Here's the run down on what's changed.

Redux Domains

  • introduces “domains” - a way to slice of your app like “Login” or “Temperature”
  • they live in App/Redux/*Redux.js
  • each contain the all redux-parts needed: action types, action creators, selectors, and reducers.
  • action creators are exported as the Actions object
  • types are exported as the Types object
  • selectors are exported as their own function names
  • the reducer is export as the reducer function
  • sub-reducer functions are buried, but are optionally exposed as functions
  • there is no default export

Store

  • moves to App/Redux/CreateStore.js
  • is only accessed by App/Redux/index.js
  • is passed the root reducer & root saga from the outside

Sagas

  • are now plural (e.g. App/Sagas/LoginSagas.js)
  • are generator functions that accept an action or (optionally) api and action
  • watchers are gone
  • the root saga now only handles takeLatest or takeEvery
  • the root saga is responsible for gluing Action Types to Sagas
  • the root saga is now a table-of-contents
  • *Sagas.js files have no default export

Selectors

  • not officially a thing previously but some of us had project with them
  • now live as exported functions inside the relevant *Redux.js
  • scope the state to that domain if possible
  • for cross-domain-cutting selectors, add additional parameters for the other domain states or consider moving to a separate domain

Other Functions

  • it makes sense to place other functions in here too … helpers, validations, business logic… remember to use pure functions!

Redux Persist

  • App/Config/ReduxPersist is now responsible for the blacklist & whitelist

index.ios.js & index.android.js

  • gutted these to remove all functionality
  • these are as pass-through as can be

App.js

  • called by index.ios.js and index.android.js
  • sets up Redux for the app by creating the store & making the Provider
  • nothing visual is done at this level
  • exists to provide a friendly buffer around React Native hot-reloading

RootContainer.js

  • the visual root — all components get added in here
  • holds things like status bar and navigation router (I’d actually like to NavigationRouter rolled in here to be honest)
@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Sep 6, 2016

That's cool man, it seems that we're using an almost identical solution for the naming of the ducks 🌻
Any hint on the issues you're facing with this approach?
On my side I had some problem handling BIG reducers, but it seems that most of the time I face this issue is just because I didn't combine the reducers enough (by creating "sub-reducers").
Tomorrow I'll take a closer look at all the changes you've made, I'm intrigued!

@ruskid

This comment has been minimized.

ruskid commented Sep 7, 2016

Wow. well done!
I am testing this commit and really like this approach. More centered, less folders.

I renamed App.js to RootContainer. and current RootContainer to just Root. just preference

@ruskid

This comment has been minimized.

ruskid commented Sep 7, 2016

What do you think about putting sagas into Redux folder and grouping all by domain? (component like)
Maybe this kind of structure:
Folders: Redux - Login (Domain Component) contains
LoginRedux.js
LoginSagas.js

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Sep 7, 2016

What do you think about putting sagas into Redux folder and grouping all by domain? (component like)
Maybe this kind of structure:
Folders: Redux - Login (Domain Component) contains
LoginRedux.js
LoginSagas.js

Just my 2 cents: from my point of view sagas are much more difficult to organize by domain.
For example there are a lot of cases where you will need to emit actions of different domains from a single saga:

export function* getMenuEntries (action) {
  try {
    const user = yield select((state) => state.auth.user)
    const [menuEntries, categories, extras] = yield [
      call(parseServiceMenu.getMenuEntries, user.place),
      call(parseServiceCategory.getCategories, user.place),
      call(parseServiceExtra.getExtras, user.place)
    ]
    yield put({ type: menuActionTypes.GET_MENU_ENTRIES_SUCCESS, menuEntries })
    yield put({ type: categoryActionTypes.GET_CATEGORIES_SUCCESS, categories })
    yield put({ type: extraActionTypes.GET_EXTRAS_SUCCESS, extras })
  } catch (err) {
    const error = err.message || err
    yield put({ type: menuActionTypes.GET_MENU_ENTRIES_FAILURE, error })
    yield put({ type: uiActionTypes.SHOW_SNACKBAR, snackbarMessage: error })
  }
}

What do you think?

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 7, 2016

Ya, I'd like to keep sagas outside for now. For the same reason as @mmazzarolo mentioned. It's quite common to trigger a "snackbar" or "alert" from sagas, whose Types is held in a different domain.

I'm totally open to renaming App and RootContainer. It's going to be tricky to agree on those names though. If it helps, the purpose of those two files are to be a "non-visual" starting point and a "visual" starting point.

I'd actually love to roll NavigationRouter into "visual".

@skellock skellock added the discussion label Sep 7, 2016

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 7, 2016

First let me say I ❤️ this so much! What a great refactor! Let's work together to make it F%(#ING FANTASTIC!

Redux Domains

  • introduces “domains” - a way to slice of your app like “Login” or “Temperature”
  • they live in App/Redux/*Redux.js
  • each contain the all redux-parts needed: action types, action creators, selectors, and reducers.
  • action creators are exported as the Actions object
    The reason I dislike this, is that you never intend for this name to be used. This causes an overuse of the as to be standard instead of exception. Take a look at this:
    image
    We might as well accept that this cannot be the name, it never will be. Export each actions as <Domain>Actions or better so, default.
  • types are exported as the Types object
  • selectors are exported as their own function names
  • the reducer is export as the reducer function
  • sub-reducer functions are buried, but are optionally exposed as functions
  • there is no default export
    📝 - The one thing everyone really wants when coding is the actions. Everything is available for tests etc, but 90% of the time we want actions, so let's make that the default export. It makes sense to me that that is exact reason why you'd have a default.

Store

  • moves to App/Redux/CreateStore.js
  • is only accessed by App/Redux/index.js
  • is passed the root reducer & root saga from the outside

Sagas

  • are now plural (e.g. App/Sagas/LoginSagas.js)
  • are generator functions that accept an action or (optionally) api and action
  • watchers are gone
  • the root saga now only handles takeLatest or takeEvery
  • the root saga is responsible for gluing Action Types to Sagas
  • the root saga is now a table-of-contents
  • *Sagas.js files have no default export
    📝 - Good call on that here. There should be no default by the nature of sagas they are all different.

Selectors

  • not officially a thing previously but some of us had project with them
  • now live as exported functions inside the relevant *Redux.js
  • scope the state to that domain if possible
  • for cross-domain-cutting selectors, add additional parameters for the other domain states or consider moving to a separate domain

Other Functions

  • it makes sense to place other functions in here too … helpers, validations, business logic… remember to use pure functions!

Redux Persist

  • App/Config/ReduxPersist is now responsible for the blacklist & whitelist

index.ios.js & index.android.js

  • gutted these to remove all functionality
  • these are as pass-through as can be

App.js

  • called by index.ios.js and index.android.js
  • sets up Redux for the app by creating the store & making the Provider
  • nothing visual is done at this level
  • exists to provide a friendly buffer around React Native hot-reloading

RootContainer.js

  • the visual root — all components get added in here
    📝 - I would like that said in comments on that file, or perhaps a README.md in the Containers folder.
  • holds things like status bar and navigation router (I’d actually like to NavigationRouter rolled in here to be honest)
    📝 - NavigationRouter is more a routes file than it is visual. Yes it is both, but I would not want any additional visual aspects. If you ask me, we went backwards a little when we had to go to RNRF, because we conflated visual with screen properties. I've had a few people agree on this front, but it's outside of our control. I have to disagree with you on further moving backwards. The navigation is very easy to find/modify as for now. In addition, we're going to have enough trouble generating new screens and adding them to navigation without further hurdles. If you see a way to move back to just routes, I'd advocate taking that in a heartbeat.

Additional notes

  • I'd rather see something like if (Config.reduxLogging) middleware.push(logger) for simplicity, but I assume you did this so it can be turned on/off during hot reloading? Not sure which one is better. Anyone else want to weigh in?
    RE: CreateStore.js
    image
  • Should we have a transform example? I think we all agree on transforms, but in here we do a transform inline in TemperatureSagas.js where it might be best to show a transform. I'm happy to help add this part if we all agree.
  • Would you like me to add the magic to createTypes in reduxsauce? I'd like to unveil that idea here. Which would take the TEMPERATURE trifecta into 1 line. Very useful in making all createTypes 66% smaller!
  • I know we dislike magic, but I have another simplification idea. This removes busywork crap in my opinion. How about something crazy like this?:
    Original:
import { createTypes, createReducer } from 'reduxsauce'

export const Types = createTypes(`
  LOGIN_REQUEST
  LOGIN_SUCCESS
  LOGIN_FAILURE
  LOGOUT
`)

export const Actions = {
  loginRequest: (username, password) => ({ type: Types.LOGIN_REQUEST, username, password }),
  loginSuccess: (username) => ({ type: Types.LOGIN_SUCCESS, username }),
  loginFailure: error => ({ type: Types.LOGIN_FAILURE, error }),
  logout: () => ({ type: Types.LOGOUT })
}

Madscience:

import {simpleCreator, build, createReducer} from 'reduxsauce'

const {Types, Actions} = simpleCreator({
  loginRequest: build('LOGIN_REQUEST', username, password),
  loginSuccess: build('LOGIN_SUCCESS', username),
  loginFailure: build('LOGIN_FAILURE', error),
  logout: build('LOGOUT')
})

export Types
export Actions

Benefits: no longer having a mismatch between types and actions, because no longer considering them 2 separate places. Am I crazy? Probably...

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 7, 2016

@GantMan woo! Thx.


Generic Names: Actions and Types

...is that you never intend for this name to be used

Kinda. This is only true for saga or containers which need to access multiple domains. For simpler consumers, this will work great:

import { Actions } from '../Redux/GantRedux'

The as keyword is used for disambiguation.

I don't dispute your claim though. In your example... ya... this is one of the downsides of going with domains.

Another alternative might be dropping the Actions "namespace" altogether. Perhaps we just leave these as normal exported functions?

import { loginRequest, loginSuccess, loginFailure } from '../Redux/LoginRedux'

We might have some name collisions with reducers inside, but nothing we can't figure out?

We also lose the ability to group our actions together & feed them into something like bindActionCreators. Something I don't really use anyway.


Export Actions as default

90% of the time we want actions

Sagas & smart components will primarily be after action creators yes.

Here's the problem I see with this though. There's a LOT going on inside these domains. Potentially 5 or 6 different things.

Rather than pick a "favorite", I made them all equal. There's no default. More like how react-native works and less like how react works.

If we were to promote Actions to default, we'd only benefit the import statement.

import Actions from '../Redux/UserRedux'

Which isn't too bad, to be honest. But now we still have Types inside and we can't export that the same way either. Maybe that's ok though?


Merging NavigationRouter inside RootComponent

You have a great point. NavigationRouter is data; disguised as a component. Blurry lines for sure.

I'm ok leaving it as is.


Logger if statement

Sure, we can change that if you want. Generally you don't want to create variables you won't use, but this will be completely immeasurable I'd wager.


Transforms Example

Already done. I moved that out of the saga and into TemperatureRedux.js. Check it out.

I don't like how we're switching on i18n to make the decision, but it's not harmful. I did have to add a mock to our Test/Setup.js though to pass the tests again. :)


createTypes templating

I'd absolutely accept a PR with templating on reduxsauce.

Now that we're moving away from a giant list of actions in 1 file, i see it as less useful though. You'll end up writing the template to use it once per file.

That's just my opinion though. The idea is kinda cool. Just need to find a nice way to reuse it.

Pitch it dude, and you can use @mmazzarolo 's gist to boost your case. Those types could definitely benefit from a template.

In the app @kevinvangelder, @carlinisaacson, and I are building, we wouldn't have a domain file with multiple request/response/failures.


Types + action creator factory

It would DRY things up sure. But now we have 2 different ways to make action creators and types.

What happens when you have an action creator that doesn't fit this format? Lets say it does some business logic to assemble the right action object. Now you're left attaching things manually to the Types and Actions objects.

Also, if we're considering promoting Actions to standard exported actions, we've got more conversion to write.

One concern I have about reduxsauce in particular is straying too far from Redux. Not only do people have to learn Redux, but they have to learn our idioms on top of that... which isn't portable.

I'm torn. I don't have the right answer.


Thank you very much for the feedback.

We can certain cherry pick things out of this commit if they're not right for Ignite.

Even if I disagree about some stuff, I'm open to implementing them.

Don't worry about hurting my feelings! Call out shit that doesn't sit right with you.

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 7, 2016

Export Actions as Default

Rather than pick a "favorite", I made them all equal. There's no default. More like how react-native works and less like how react works.

I think there is a favorite, though. Making them equal when one is commonly used is actually the complaint. Tests are the only ones that care for everything.

Types + action creator factory

What happens when you have an action creator that doesn't fit this format? Lets say it does some business logic to assemble the right action object.

It could look like so:

const {Types, Actions} = simpleCreator({
  somethingSpecial: (x) => (x === 'taco') ? buildObject('TACO') : buildObject('NACHO')
})

Both types are created via buildObject, but the action creator would activate only one of them. Or get rid of buildObject and just return an obect with simpleCreator doing the work on Types.
simpleCreator could take a second argument, which would be a list of types not defined by build calls. The types + creators always felt like busywork to me. This is nice.
Without buildObject

const {Types, Actions} = simpleCreator({
  somethingSpecial: (x) => (x === 'taco') ? {type: 'TACO'} : { type: 'NACHO'}
}, ['TACO', 'NACHO'])

As for reduxsauce straying from Redux
I see reduxsauce simplifying redux. Anyone who knows redux, would quickly pick up reduxsauce and appreciate it. Anyone coming to redux for the first time would find reduxsauce easier to read. Redux is too minimalistic, which is why it causes so much boilerplate. Yes, every time we hide complexity, we make a level of ignorance, but not everyone should be a mechanic to drive their car. I feel our abstractions have been fair in reduxsauce.

@mmazzarolo

This comment has been minimized.

Contributor

mmazzarolo commented Sep 7, 2016

This is another awesome discussione in my opinion, the last two posts are worth a book. 📖

Another alternative might be dropping the Actions "namespace" altogether. Perhaps we just leave these as normal exported functions?
import { loginRequest, loginSuccess, loginFailure } from '../Redux/LoginRedux'
We might have some name collisions with reducers inside, but nothing we can't figure out?
We also lose the ability to group our actions together & feed them into something like bindActionCreators. Something I don't really use anyway.

I tried it and I would stay away from it:

  1. You lose the ability to feed them do mapDispatchToProps
  2. It might result in an infinite and boring list of imported actions, especially in bigger containers
  3. You'll waste a precious variable name just for passing it to mapDispatchToProps
  4. ...The variable name is precious (in my opinion) because you won't be able to do something like const { loginRequest } = this.props
  5. In bigger sagas and in the root one you'll end up using a lot of different actions without knowing their context (you'll have to scroll to the import every time).
@skellock

This comment has been minimized.

Contributor

skellock commented Sep 7, 2016

Ya, that's my preference too is to keep it an object. Since nothing is currently default, we could also export that same object as default, but I've never seen people do that.

So, from an interface point-of-view, @GantMan you have an issue with:

  • Actions being a named export and not default
  • Types & Action Creators not going far enough to reduce boilerplate

Did you want to sit on this PR for another week and give it time to percolate?

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 7, 2016

I say we fix up the Actions export, I'm happy to do that.

The Types & Action Creators change is somewhat extreme, and we should evaluate if it's realistic or not. If it is, I say we get active on it, if it's not, then the PR is 1 fix away from gold to me!

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 7, 2016

Results from the Great Gathering of Ignite Mages

1 - Actions and Types are now <DOMAIN>Actions and <DOMAIN>Types
2 - Transforms move to separate folder
3 - Make Actions the default export from any Redux file
4 - README.md in containers for RootContainer.js
5 - Experiment with simpleCreator madness to find a loveable API

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 8, 2016

Changes we discussed

  • Action Creators are now the default thing exported from *Redux.js files.
  • Types are now exported with a domain prefix.
  • Added App/Transforms with convertToKelvin and a readme
  • Updated the readme in Containers

Ninja edits we didn't discuss

  • Added a readme to Lib
  • Cleaned up a few container screens to move NavigationActions out of mapDispatchToProps. These things aren't redux-related and are totally fine to be called directly.

Things I'm avoiding

  • The simpleCreator thing to wrap Types and Action Creators.
@skellock

This comment has been minimized.

Contributor

skellock commented Sep 8, 2016

Ok, so here's the thing for about that simpleCreator thing.

Yes, you're right. There's an opportunity to DRY things up a bit.

But this comes at the expense of blurring the lines between what an action creator is and what our types are with our own custom API.

When I look at this file and i see a line that creates Types and another one that creates Action Creators, it's crystal clear that these two things are different. And if I'm already familiar with Redux, i'm kind of expecting to see that.

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 8, 2016

Just had a meeting with @GantMan . We came up with something we're both happy with.

createActionTypeSet. It takes an object and creates both your Types and your Action Creators out of it.

Keys will become the names of the functions inside Actions as well as the names of the types (once we convert to SCREAMING_SNAKE_CASE).

Values are 1 of the following options:

  • null meaning the action has no additional properties (optionally [])
  • array of strings meaning the action creator will accept those additional inbound parameters
  • function meaning you'll write the creator yourself

This will cut down the boilerplate significantly (@GantMan's requirement) and doesn't stray too far from Redux's spirit (@skellock's requirement).

const { Types, Actions } = createActionTypeSet({
  loginRequest: ['username', 'password'],
  loginSuccess: ['username'],
  loginFailure: ['error'],
  logout: null, // or []
  custom: function (var1, var2) {
    return { type: 'CUSTOM', crazyVar: var1 + var2 }
  }
})

export const LoginTypes = Types
export default Actions

Building now.

@skellock

This comment has been minimized.

Contributor

skellock commented Sep 8, 2016

Called it createActions which was @GantMan's original suggestion after I butchered it several times.

https://github.com/skellock/reduxsauce#createactions

Here's the full API in action. The custom one at the end is just the escape hatch. We have several projects on the go that wouldn't even use that.

import { createActions } from 'reduxsauce'

const { Types, Creators } = createActions({
  loginRequest: ['username', 'password'],
  loginSuccess: ['username'],
  loginFailure: ['error'],
  logout: null,
  custom: (a, b) => ({ type: 'CUSTOM', total: a + b })
})

@skellock skellock force-pushed the redux-remix branch from b9a35b7 to a60f01b Sep 8, 2016

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 8, 2016

OOOOOH

@kevinvangelder

This comment has been minimized.

Member

kevinvangelder commented Sep 9, 2016

❤️

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 9, 2016

I think we're all in agreement this is ready for merge. A most epic update. Will merge soon if no objections.

@GantMan GantMan merged commit 402afb8 into master Sep 9, 2016

@GantMan GantMan deleted the redux-remix branch Sep 9, 2016

@GantMan

This comment has been minimized.

Member

GantMan commented Sep 9, 2016

done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment