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

RFC: Redux Starter Kit #2859

Closed
timdorr opened this issue Mar 1, 2018 · 56 comments

Comments

@timdorr
Copy link
Member

@timdorr timdorr commented Mar 1, 2018

Based on comments over in #2858 and earlier in #2295, it sounds like a preconfigured starter kit of Redux core + some common middleware + store enhancers + other tooling might be helpful to get started with the library.

I've started by reserving a package name (I'm not dead set on it, though) and we can start to fill in the blanks there. I would like to maintain the contents of that package here, so we don't have to set up another repo or anything. We may want to investigate a monorepo, but I don't think there's a pressing need currently.

One thing I'd like to investigate at the same time: splitting combineReducers to its own package. It's the one thing that gets prescriptive about how to structure and operate a store. It's obviously not the only way, but I worry many folks don't see past it.

As for what goes in a starter package, the short list in my mind includes one of the boilerplate reduction libraries (or something we build), popular middleware like thunks and sagas, and helpful dev tooling. We could have a subset package for React-specific users. In fact, I started a Twitter poll to find out if that's even necessary (please RT!).

I don't think we need to build a Create React App-like experience with a CLI tool and all that jazz. But something out of the box that gives a great dev experience with better debugging and less code.

@timdorr timdorr changed the title Redux Starter Kit RFC: Redux Starter Kit Mar 1, 2018
@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

YES YES YES! I HAVE MANY OPINIONS AND WISHES FOR THIS! But I'll restrain myself and let the discussion get going some first.

One wishlist item I'd have that I'm not sure is feasible is built-in HMR for reducers automatically added in a createReduxStore() function. Unfortunately, my knowledge of Webpack and some experimentation says that's not very feasible, because you have to have hardcoded paths to reducer files in the Webpack module.hot.accept() callbacks. Not sure how Parcel or other bundlers handle that.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

So, initial request for feedback from everyone:

  • What are the key requirements we would want to satisfy with this package?
  • What existing packages would you suggest to help fulfill those requirements?
  • How can we best combine those packages to keep things simple for the end user?
@theKashey

This comment has been minimized.

Copy link

@theKashey theKashey commented Mar 1, 2018

Lets name 3 key points, where possible variants will be isolated from each other.

  1. First point. Reducer managers
    1.0 Vanilla (it IS the case here?)
    1.1 Immer.js
    1.2 Coredux
    1.3 100500 more packages

  2. Second point. Middlewares
    2.0 Vanilla (is it the case?)
    2.1 Sagas
    2.2 Epics
    2.3 100500 more packages.

  3. Third point. React integration
    3.0. Vanilla (it IS the case here?)
    3.1. Subspace
    3.2 Restate
    3.3 100500 more packages.

4th, bonus, point
4.0 Testing all this stuff in a boilerplate.

It is impossible to pick "the chosen ones", for example I love and use both Sagas and Epics, but possible to provide some "cooking receipts" and recommendations for combinations.

For example most of redux users aware only about middlewares. A small part also doing something with reducers(ok lots heard about immutable.js), just a few ones enhance React integration.

Starter Kit could just help build for full and rich approach from the beginning. Or one need more that one Starter Kit.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

Asked for feedback on setup/boilerplate pain points on Twitter: https://twitter.com/acemarke/status/969040835508604929

@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 1, 2018

Copying my response on twitter:

I’ve explicitly stayed away from adding redux boilerplate as IMO it adds complexity when introducing ‘your way’ of doing redux to another person. I would love more prescriptive conventions enforced via code to make redux a more portable skill.

To that end, having a create-redux-app style repo - however designed more as a library than a framework - especially one explicitly endorsed by Redux maintainers of best and recommended practices, I think would go a long way to solidifying and adding clarity on proper Redux usage.

@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 1, 2018

We've been talking a bit about this on Reactiflux, wanted to copy over a consolidation of desires for inclusion in this thread, some of this overlaps with the points you already listed @timdorr (so it's an enthusiastic confirmation).

  1. wrapper around createStore with better defaults, i.e. easier way to add middleware, redux dev tools
  2. built in immutable helper (immer?)
  3. createReducer built in (the standard "reducer lookup table" utility)
  4. reselect built-in
  5. action creator that adheres to FSA
  6. async action support / side effect support (flux, saga, what?)
@cassiozen

This comment has been minimized.

Copy link

@cassiozen cassiozen commented Mar 1, 2018

Why not officially backing something that already exists, like Henrik Joreteg's redux-bundler?

https://github.com/HenrikJoreteg/redux-bundler

It's concise, opinionated (in a good way), allows for reuse of redux-related functionality across applications and still gives a nice solution for async handling.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

It's a fascinating-looking project, and one that I really want to investigate further, but it's also out of scope for what I want to do with this task.

What I really want to accomplish here is:

  • Setting up a Redux store with a couple lines of code and minimal configuration choices needed (the Redux equivalent of the "zero-config JS" tagline Webpack and Parcel have been using).
  • Similar to CRA, config stuff is basically hidden, with good defaults out of the box
  • Inclusion of carefully selected packages to improve dev experience and, yes, "reduce boilerplate". For example, Immer would be a great choice because it drastically simplifies writing immutable reducer logic and freezes your state in dev. (My only hesitation there is that the appearance of code that's "mutating" would be confusing.)

I want to keep this effort fairly tightly scoped. I've been chatting with @hswolff and @nickmccurdy over in Reactiflux the last couple hours, and while the discussion has been great, we've already run through a dozen overlapping use cases and ideas that could really over-complicate things.

I want to get something useful, valuable, and viable put together without too much bikeshedding, and then have other discussions off to the side (like an API for making Redux "bundles" or "modules", etc).

@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 1, 2018

To kick off some initial boilerplating of APIs here's a basic idea for 1-3 of my list I posted above:

// #1
export function createStore({
    // If an object is given it's passed to combineReducers
    // otherwise it's just used directly.
    reducer: Object<String, Function> | Function,
    // Middleware is built-in and accepts an array of middleware functions.
    middleware: Array<MiddlewareFunction>,
    // Built-in support for devtools.
    // Defaults to NODE_ENV !== production
    devTools: boolean,
    // Same as current createStore.
    preloadedState,
    // Same as current createStore.
    enhancer,
}) {};

// Re-exported for easy usage.
export function combineReducers() {}

// #2
// Re-export immer as the built-in immutability helper.
import immer from 'immer';
export const createNextState = immer;

// #3
// Export an already written version of createReducer .
// Same thing as https://redux.js.org/recipes/structuring-reducers/refactoring-reducers-example#reducing-boilerplate
export function createReducer() {}
@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

Yep, I dig that. I'd then expect that it would either always add thunk() at the front of the middleware list, or possibly only if you didn't supply any middlewares yourself. We might not even want to allow the user to specify enhancers here. We'd add applyMiddleware() automatically if there's any middleware, and use the composeWithDevtools function from redux-devtools-extension if devTools : true. For purposes of this "easy setup" thing, I think we may want to eliminate "enhancers" from the things the user can configure.

@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 1, 2018

+1 on eliminating enhancers.

On your point of thunk being automatically added, we should be able to have our cake and eat it too via exporting the function we'd use to add default middleware:

export function createDefaultMiddleware(...additional) {
    return [thunk(), ...additional],
}

// Within a user's application
createStore({
    // By default it's just createDefaultMiddleware()
    // However if you want to add any other middleware you can.
    middleware: createDefaultMiddleware(any, other, middleware)
})
@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 1, 2018

Yeah, that's roughly what I was thinking. My main concern is figuring out if the user accidentally tries to supply their own instance of thunk.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

I decided to play around with things a bit this evening. Here's a first cut. Thoughts?

// configureStore.js
import {createStore, compose, applyMiddleware, combineReducers} from "redux";
import { composeWithDevTools } from 'redux-devtools-extension';
import createNextState from "immer";

import isPlainObject from "./isPlainObject";

import thunk from "redux-thunk";


const IS_DEVELOPMENT = process.env.NODE_ENV !== "production";

export function createDefaultMiddleware(...additional) {
    return [thunk, ...additional];
}

export function configureStore(options = {}) {
    const {
        reducer,
        middleware = createDefaultMiddleware(),
        devTools = IS_DEVELOPMENT,
        preloadedState,
    } = options;

    let rootReducer;

    if(typeof reducer === "function") {
        rootReducer = reducer;
    }
    else if(isPlainObject(reducer)) {
        rootReducer = combineReducers(reducer);
    }
    else {
        throw new Error("Reducer argument must be a function or an object of functions that can be passed to combineReducers");
    }

    const middlewareEnhancer = applyMiddleware(...middleware);

    const storeEnhancers = [middlewareEnhancer];

    let finalCompose = devTools ? composeWithDevTools : compose;

    const composedEnhancer = finalCompose(...storeEnhancers);

    const store = createStore(
        rootReducer,
        preloadedState,
        composedEnhancer
    );

    return store;
}

export function createReducer(initialState, actionsMap) {
    return function(state = initialState, action) {
        const {type, payload} = action;

        return createNextState(state, draft => {
            const caseReducer = actionsMap[type];

            if(caseReducer) {
                return caseReducer(draft, payload);
            }

            return draft;
        });
    }
}
export {createNextState, combineReducers};

And the obligatory example todo app:

import {configureStore, createReducer} from "./configureStore";

const ADD_TODO = "ADD_TODO";
const TOGGLE_TODO = "TOGGLE_TODO";
const SET_VISIBILITY_FILTER = "SET_VISIBILITY_FILTER";

function setVisibility(state, newFilterType) {
    return newFilterType;
}

const visibilityReducer = createReducer("SHOW_ALL", {
    [SET_VISIBILITY_FILTER] : setVisibility
});


function addTodo(state, newTodo) {
    state.push({...newTodo, completed : false});
}

function toggleTodo(state, payload) {
    const {index} = payload;

    const todo = state[index];
    todo.completed = !todo.completed;
}

const todosReducer = createReducer([], {
    [ADD_TODO] : addTodo,
    [TOGGLE_TODO] : toggleTodo
});


const preloadedState = {
    todos: [{
        text: 'Eat food',
        completed: true
    }, {
        text: 'Exercise',
        completed: false
    }],
    visibilityFilter : 'SHOW_COMPLETED'
};


const store = configureStore({
    reducer : {
        todos : todosReducer,
        visibilityFilter : visibilityReducer,
    },
    preloadedState,
});

const exercise1 = store.getState().todos[1];

store.dispatch({type : "TOGGLE_TODO", payload : {index : 1}});

const exercise2 = store.getState().todos[1];

console.log("Same object: ", exercise1 === exercise2);

store.dispatch({type : "SET_VISIBILITY_FILTER", payload : "SHOW_COMPLETED"});

console.log(store.getState());
@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 2, 2018

I think this is a solid start and a great beginning to the lib being discussed in this thread. It covers points 1,2,3, and 6 from my list of desired feature above.

Iterating on top of this would be great, but this is a solid foundation.

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Mar 2, 2018

Rather than focusing on the implementation, I'd like us to focus on the DX. What are the exports/APIs and how are they used? I think it's a great chance to get past just the configuration and also look at how folks use the store. Some random ideas:

Easier Reducer/Action pairs:

Make it super-easy to pair up your reducers and actions along with their state output. This is rough, but here's the basic idea:

import { createStore, createActionPack, combineActionPacks } from 'redux/starter'

const counter = createActionPack({
  increment: counter => counter + 1,
  decrement: counter => counter - 1,
  set: (_, value) => value
}, 0)

const posts = createActionPack({
  load: async () => await fetch('/posts'),
  create: async (state, text) => state.push(await fetch('/posts/new', { body: { text } }))
}, [])

const store = createStore(
  combineActionPacks(
    counter,
    posts
  )
)

store.dispatch(counter.increment())
store.dispatch(counter.set(42))

await store.dispatch(posts.load())
await store.dispatch(posts.create('First!'))

Store Singleton

This one I'm not sold on by any means, but I was thinking back to the days when we had history singletons in React Router. It wasn't perfect, but it provided some value.

import { store, configureStore } from 'redux/starter'

configureStore(reducer, middleware, initialState)

store.dispatch(someAction())
store.getState()

Configurability via import

I'll save my wackiest for last. But what if we allowed configuration by importing files. We'd need some behind the scenes global passing, which gets messy/troublesome. But again, the focus is on the user, not our side.

import { createStore } from 'redux/starter'
import 'redux/starter/devtools'
import 'redux/starter/logger'
import 'redux/starter/immutable'

// OR!

import { createStore } from 'redux/starter/developer'
import { createStore } from 'redux/starter/production'

For that second example, I can see users either aliasing to that package in their build tooling or doing something like what React does.

OK, that's it from me for now. Again, I want to be thinking user-first. Whatever dirty, ugly hacks we have to make to implement this is irrelevant. The whole point is a great DX with less code written and less configuration needed.

@tannerlinsley

This comment has been minimized.

Copy link

@tannerlinsley tannerlinsley commented Mar 2, 2018

Love these ideas thus far. My only wish for whatever this turns out being would be graceful access to the lower level, standard Redux api. There needs to be a natural path for people to customize or eject. Whether that is in a few days, or a year or two, it should be simple. Lock-in is one of the problems I see (from my own attempt at this with Jumpstate and from more recent libraries like Rematch). The more that we can avoid that, the better taste people will have in their mouth :)

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

Yeah, I've seen quite a few util libs that do stuff like that "action pack" (at least as far as the "write some reducers, turn the keys into action types and action creators" aspect), and I was considering throwing that in as an additional item - just hadn't gotten there yet. Not sure about the "combineActionPack" aspect.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

Had some additional discussion with @hswolff last night. I'd like to consider adding/re-exporting the selectorator library, which appears to be a superset of the Reselect API with some additional useful benefits:

import createSelector from 'selectorator';

// selector created with single method call
const getBarBaz = createSelector(['foo.bar', 'baz'], (bar, baz) => {
  return `${bar} ${baz}`;
});

That would help cut down on a lot of the hand-written "simple input" selectors, like const selectTodos = state => state.todos.

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Mar 2, 2018

I'm not sure I see the benefit of adding a selector library. We would have to also be providing some opinion about state shape that is enabled by it.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

It's at least partly just so there's one less item the user has to install into package.json.

@tannerlinsley

This comment has been minimized.

Copy link

@tannerlinsley tannerlinsley commented Mar 2, 2018

I think a selector library would be awesome.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

Also relevant: the recent article on the apollo-boost starter kit

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 2, 2018

If we can extend react-redux's connect's API to use reselect (or a similar selector library) when the mapStateToProps is an Array, we could encourage the use of selectors and make it very convenient without breaking back compatibility. I don't think this library should be coupled with React or react-redux necessarily, but maybe we could handle that extensibility through an option for react-redux or provide another package wrapping the starter kit with react-redux support.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 2, 2018

Yeah, I'm not looking to do anything with React-Redux for this initial MVP.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 2, 2018

In that case is there a good way to add an abstraction for selectors at the Redux level or is this something we could only do effectively on top of react-redux? The problem is we wouldn't want this to be incompatible with react-redux, so I guess if that's not possible we should ditch selectors for now.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 3, 2018

Should this be a part of the main redux package as a separate import (so this is completely back compatible) or would you prefer a monorepo with multiple packages? I think the former would be enough, we could always use another file export without creating a new package if it gets too big.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 3, 2018

I'd kinda like to play around with this as a separate repo/package to start with, really.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 3, 2018

It may be easier to use the existing tooling setup in this repo since merging it back in the future could be error prone. We also probably don't need different syntax support because we're not making this React specific.

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Mar 3, 2018

If it's going to be one single file with some pre-configuration, then keeping it here would be easiest. Because of the dependencies, it should be a separate package. We don't want to saddle existing apps with those.

Again, I don't think we need to go monorepo, but we can have a folder for specific packaged builds if we start building more than one. Just a starter folder for now with its own package.json should do the trick.

@hswolff

This comment has been minimized.

Copy link

@hswolff hswolff commented Mar 3, 2018

I like the idea of keeping this local to the main redux package if it's only using code local to redux itself. I get more hesitant if we start to pull in external dependencies to support the ideas expressed here.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 3, 2018

I think some external dependencies like immer, thunk/promise/saga, and reselect would be invaluable to our goals here and worth the extra dependencies for the starter, though we may not want those dependencies on the main redux package itself.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 3, 2018

So, uh... I went and did a thing:

Things that are different from the snippet earlier:

  • I threw in an enhancers config option, just because
  • I added the selectorator dependency and exported createSelector

Thoughts?

@UnderpantsGnome

This comment has been minimized.

Copy link

@UnderpantsGnome UnderpantsGnome commented Mar 3, 2018

As someone that just taught myself Redux (and React) in the last couple weeks, I can't wait to see the final product. I'm sure I'm going to have a ton of stuff to clean up in my code.

😏 👍

@theKashey

This comment has been minimized.

Copy link

@theKashey theKashey commented Mar 5, 2018

So, as long idea is to simplify redux usage, I could propose 2 moments:

  1. Add build-in function to literally expect(mapStateToProps(state)).toEqual(mapStateToProps(state)). Just to encourage people to create a better code. I never saw tests like this, but not-very-pure mapStateToProps is a one of redux problems.

  2. Consider adoption something like memoize-state. It is like Immer.js for selectors. The key point - the customer could just write mapStateToProps or selector in the form he prefers, and get it memoized. It even could solve this reselect issue with "Sharing Selectors with Props Across Multiple Component Instances" without loosing "common" memoization between instances. Just double memoize the stuff

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 5, 2018

  1. I see what you mean in terms of recommending pure selectors, but an impure idempotent selector could still pass that test. I like the second suggestion more.
  2. That sounds cool, so we'd just wrap selectors in the same way we'd wrap reducers for immer, right?

Still, as we discussed above, this is more of a react-redux thing than something that would be in the starter package which is agnostic to React.

@joshwcomeau

This comment has been minimized.

Copy link

@joshwcomeau joshwcomeau commented Mar 6, 2018

This is really cool!

I have one request / bit of feedback, though; I'd rather if Immer was not used implicitly.

Immer is great. It's become one of my favorite utilities. But even when using it explicitly (produce(baseState, draftState => ...), I worry about when other people who aren't familiar with it view my code, and don't realize that I can only do this mutative stuff because of Immer superpowers.

The way the API looks now, I can imagine newcomers not realizing that redux state should be immutable. I think that misunderstanding would bite them hard when they try and use Redux without this starter kit.

I see that createNextState is already an export, so why not encourage users to use it explicitly:

import {createReducer, createNextState} from "@acemarke/redux-starter-kit";
 
function addTodo(state, newTodo) {
    return createNextState(state, draftState => {
        draftState.push({...newTodo, completed : false});
    });
}

(apologies if I've misunderstood how createNextState is intended to be used! Haven't dug into it at all)

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 6, 2018

See reduxjs/redux-toolkit#5.

The way the API looks now, I can imagine newcomers not realizing that redux state should be immutable. I think that misunderstanding would bite them hard when they try and use Redux without this starter kit.

In my opinion, we should recommend against using Redux directly in favor of this package when it is complete because of #2858. Basically we want to either warn or safeguard users from all mutations by default, but we can't easily do this in core without breaking things, so we should recommend against using core except in edge cases where the starter kit is not sufficient. This is similar to how React recommends CRA and Redux recommends react-redux when using React.

I see that createNextState is already an export, so why not encourage users to use it explicitly:

Not a bad idea, but I think we're more in favor of being implicit here especially because we're not generating source code and the addition of immer is back compatible.

@codedavinci

This comment has been minimized.

Copy link
Contributor

@codedavinci codedavinci commented Mar 19, 2018

I see a lot of people recommending to use libs like immer.js, which from my humble opinion consists in unwittingly deviate the motivation of learning and respecting the store requirements.

We all know that the store can't be mutated, thus, I'd much rather have a validating mechanism where it checks the newState instance against ( === ) the previousState and if it's the same instance throw an error.

This way everybody would be enforced to learn the store's rules and requirements.

PS: I truly understand the benefit of immer and I have absolutely nothing against it, but we are reaching a time where people are finally understanding the concept, it'd generate a huge source of confusion, and technically saying, if we benchmark it, we'd be doing nothing but adding an extra layer of complexity just to loosen up the library.

Personal Suggestions:

The logger, devTools and Thunk should be added as default of the Starter, the rest I believe is more of a personal choice.

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 19, 2018

We all know that the store can't be mutated, thus, I'd much rather have a validating mechanism where it checks the newState instance against ( === ) the previousState and if it's the same instance throw an error.

There are cases where depending on the action you wouldn't want the state to change, so you'd return the same store instance to save memory and prevent any false positive diffs. Even if we got around this issue it seems like Redux doesn't want a validator in core and I think Immer is an okay safeguard for this personally. I do agree that it's kind of defeating the purpose of learning Redux to an extent, but I see so many developers learning Redux that don't want to learn pure FP or aren't aware of it.

if we benchmark it, we'd be doing nothing but adding an extra layer of complexity just to loosen up the library.

As far as I'm aware, the way that Immer is implemented the Proxy only has performance overhead if it's actually mutated, so typical immutable update reducers shouldn't have a performance overhead. I think this is an okay balance.

The logger, devTools and Thunk should be added as default of the Starter, the rest I believe is more of a personal choice.

There are some other things I'd like to consider removing (though I personally don't see a huge advantage to the logger over the developer tools that makes up for its performance overhead), see reduxjs/redux-toolkit#19. However I'm not against adding more things if they're back compatible like our current usage of Immer and Thunk.

@codedavinci

This comment has been minimized.

Copy link
Contributor

@codedavinci codedavinci commented Mar 19, 2018

There are cases where depending on the action you wouldn't want the state to change, so you'd return the same store instance to save memory and prevent any false positive diffs. Even if we got around this issue it seems like Redux doesn't want a validator in core and I think Immer is an okay safeguard for this personally. I do agree that it's kind of defeating the purpose of learning Redux to an extent, but I see so many developers learning Redux that don't want to learn pure FP or aren't aware of it.

@nickmccurdy I totally understand your point of view, and I do agree that we shouldn't have a learning obstruction due to some concepts of FP, Mutation and etc... But as an educator I strongly believe in not sugar coating the apprentice by hiding the complexity from them. Anyway, it's just a matter of taste, I am a pro learning and forwardness, and looking at this proposal, my instincts somehow tells me that we'd be moving forward with acceptance, but regressing with standards. Just in note, think of all content in the web right now teaching Redux Standards and how it's finally picking up.

As far as I'm aware, the way that Immer is implemented the Proxy only has performance overhead if it's actually mutated, so typical immutable update reducers shouldn't have a performance overhead. I think this is an okay balance.

When I mean adding a layer of complexity, I mean Immer has never been there and now it could possibly be, we'd have to support it in the library, test against, evaluate perfomance issues and last but not least this section here, makes me apprehensive:

Immer.js - Auto freezing
Immer automatically freezes any state trees that are modified using produce. This protects against accidental modifications of the state tree outside of a producer. This comes with a performance impact, so it is recommended to disable this option in production. It is by default enabled. By default it is turned on during local development, and turned off in production. Use setAutoFreeze(true / false) to explicitly turn this feature on or off.

And all of a sudden now we also need to support features added in Development / Prod. It's just a good amount of work and worrisome for not much to trade off, but again, this is just my personal opinion.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 19, 2018

@codedavinci : the point of this redux-starter-kit library is to add useful tools and abstraction on top of the "basic" Redux store setup process. Also, note that this is not going into the Redux core itself, but rather into a separate library that we would like to make an "official" part of the Redux org eventually.

All your existing Redux code will work. All existing tutorials will still be fine. We're just trying to add an officially supported simpler approach to setting up and using Redux.

@codedavinci

This comment has been minimized.

Copy link
Contributor

@codedavinci codedavinci commented Mar 19, 2018

@markerikson I understand the goal of having the starter kit and I am fully down for everything mentioned above, just a bit resistant of having a lib like Immer.js that changes the concept, of the library, people will eventually learn the starter-kit patterns and fully ignore the core concepts. No store mutation is the motto, Redux's been always selling it, it's part of the brand.

PS: I fully understand that Immer.js doesn't mutate anything, but rather helps you not to, I am only and just talking about syntactically standards allowed such as push, splice...

Maybe, It's just myself being resistant against changes :)

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 20, 2018

@codedavinci : yeah, I understand the point of the concerns. Still, one of the common complaints about Redux is the pain of writing proper immutable update code, and Immer is simply the best library I've seen for simplifying those updates.

@codedavinci

This comment has been minimized.

Copy link
Contributor

@codedavinci codedavinci commented Mar 20, 2018

@markerikson, I actually did some digging into the immer and I noticed it's very light, simple and efficient. I am actually sold :) .

Which repo are you drafting things up ? those:

https://github.com/markerikson/redux-starter-kit
https://www.npmjs.com/package/@acemarke/redux-starter-kit
https://unpkg.com/@acemarke/redux-starter-kit@0.0.1/

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 20, 2018

Yep, that's it.

@nickmccurdy has a bunch of PRs waiting for discussion and merger, mostly around tooling, but also some discussion about how we want to focus the project longer-term. I need to get around to looking at those, but have been busy with other stuff the last couple weeks (including the conference talk I'm giving tomorrow).

@codedavinci

This comment has been minimized.

Copy link
Contributor

@codedavinci codedavinci commented Mar 20, 2018

Great progress @nickmccurdy & @markerikson 👍 I love the simplicity and effectiveness of it. @nickmccurdy noticed you're very active on the project, I'd love to have a chat with you about some ideas, let me know if you're up for it ! :)

@nickmccurdy

This comment has been minimized.

Copy link
Contributor

@nickmccurdy nickmccurdy commented Mar 20, 2018

Sure thanks, it's probably best to start an open discussion in a new issue but you can reach me directly via nick@nickmccurdy.com or on Discord.

@abradley2

This comment has been minimized.

Copy link

@abradley2 abradley2 commented Aug 2, 2018

+1 for splitting out combineReducers. I used it for the longest time and because if it I didn't really think deeply about how my store operated.

I currently like to set up my reducer function along the lines of:

function reducer(prevState, event, handledNavigation = false) {
  if (event.type === 'NAVIGATION' && !handledNavigation) {
    return reducer(onNavigate(prevState, event), event, true);
  }

  const pageReducer = pageReducers[prevState.activePage];

  const nextSessionState = sessionReducer(prevState.sessionState, event);
  const nextPageState = pageReducer(prevState.pageState, nextSessionState, event);

  return {
    activePage: prevState.activePage,
    pageState: nextPageState,
    sessionState: nextSessionState,
  };
}

Which is something combineReducers doesn't really let me do.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Aug 2, 2018

@abradley2 : we're not planning to remove combineReducers from the Redux core, if that's what you're asking. But yes, we've always said that combineReducers is a utility for the most common use case, and that you are encouraged to write custom reducer logic for your own situation.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Dec 16, 2018

We've got the redux-starter-kit package name , and the repo has been moved into the org at https://github.com/reduxjs/redux-starter-kit . Gonna close this thread. Any further discussion can happen over in that repo.

Also, I'm starting to work on adding mutation and serializability checks by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.