Redux-toolkit: createDefaultMiddleware Semantics

[hnordt]

I would like to suggest a change in the way we support custom middleware because createDefaultMiddleware, at least for me, sounds confusing. You are not actually creating a default middleware, you are applying your own middleware while keeping the library defaults.

Instead of:

const middleware = createDefaultMiddleware(logger);
const store = configureStore({ middleware });

We could support passing a function to middleware and receiving the default middleware as a param:

const middleware = defaultMiddleware => [defaultMiddleware, logger];
const store = configureStore({ middleware });

This also allows the consumer to control the order of defaultMiddleware in the chain, which might be useful for some people.

defaultMiddleware would be an object, so in the future we if add more middlewares we could let the user choose the ones he wants:

const middleware = ({ thunk, saga }) => [thunk, saga, logger];
const store = configureStore({ middleware });

The implementation for middleware argument would be something like:

export function configureStore(options = {}) {
    const {
        reducer,
        devTools = true,
        preloadedState,
        enhancers = [],
    } = options;

    let { middleware } = options;
    middleware = parseMiddleware(middleware);

    // ..
}

const defaultMiddleware = { thunk, saga }

function parseMiddleware(middleware) {
    if (middleware === undefined) {
      // The order for our own defaultMiddleware doesn't matter, so that's not an issue.
      return Object.values(defaultMiddleware)
    }

    if (Array.isArray(middleware)) {
        return middleware
    }

    if (typeof middleware === "function") {
      // Here we need to check for plain objects. The reason is that defaultMiddleware is
      // an object to support destructuring when the consumer wants to select specify
      // middlewares from the defaultMiddleware.
      // But if the consumer uses defaultMiddleware as is, we need to convert it
      // to an array.
      // The order for our own defaultMiddleware doesn't matter, so that's not an issue.
      return middleware(defaultMiddleware)
        .reduce(
          (result, middleware) =>
             isPlainObject(middleware)
               ? [result, ...Object.values(middleware)]
               : [result, middleware],
          []
        )
    }

    throw new Error("`middleware` should be an array or function")
}

There is a potential issue where users could think that they would be allowed to pass middleware as objects:

const apiMiddleware = { todoMiddleware, userMiddleware, ... }

const store = configureStore({
  middleware: defaultMiddleware => [defaultMiddleware, apiMiddleware]
})

It's not really an issue as long as you don't expect the order of apiMiddleware to be maintained. But that's something I didn't like, I don't know how to improve this. Help is welcome.

[nickmccurdy]

I would like to suggest a change in the way we support custom middleware because createDefaultMiddleware, at least for me, sounds confusing. You are not actually creating a default middleware, you are applying your own middleware while keeping the library defaults.

Totally agree, I think it's kind of a confusing name for a function that prepends to a list.

I like your approach better, but having a function to configure this feels complex and it may be unclear how to write the function. I think it would be beset for us to have defaultMiddleware as an array that is the default value of middleware, then users can replace it or ever concat it if necessary. Besides simplicitly, this has the advantage of maintaining a strict ordering of middleware while allowing the user to change the order of the defaults.

[hnordt]

@nickmccurdy makes sense. We could even export defaultMiddleware and then the user could do something like:

import { configureStore, defaultMiddleware } from "redux-starter-kit"

const store = configureStore({
  middleware: [...defaultMiddleware, logger]
})

[nickmccurdy]

Exactly what I was thinking. 😄 I've seen configs like this before and I like how straightforward it is.

[hnordt]

@markerikson should I make a PR?

[nickmccurdy]

Sure, I'll review it

[hswolff]

I'm not a fan of having this be a variable as it could allow external actors from modifying the array leading to unexpected behavior.

Perhaps a compromise is getDefaultMiddleware()?

[nickmccurdy]

We could freeze the variable.

[hswolff]

Unclear what that means.

[hnordt]

@hswolff I think he wanted to say freeze. Object.freeze()

[hnordt]

@hswolff getDefaultMiddleware() sounds like a great idea to me.