Add even more runtime sanity checks

[markerikson]

I already added checks for mutation and serializability. The other category of things that often comes up is side effects in reducers. Had some ideas on that front tonight:

• https://github.com/angular/zone.js/ monkey-patches every source of async behavior in the environment, and lets you create "zones" that monitor async behavior. Per Section 5, Use Case 1.a in the "Zone Primer" doc, warning about async behavior is a perfectly valid use case for Zone.js
• The other similar concern is randomness. Obviously can't check for everything, but maybe look into monkey-patching Math.random the same way? (Was specifically helping someone in Reactiflux tonight who was generating random color values in a reducer.)

I figure I can turn this into another validation middleware. The biggest issue is that I can imagine someone putting some other async middleware after ...getDefaultMiddleware(), and this one throwing errors because those happened while next(action) was running.

[denisw]

Monkey-patching sounds heavily intrusive. Should redux-starter-kit reach that far? I certainly wouldn't expect a Redux toolbox to do that kind of stuff.

[markerikson]

This would be dev-only checks, same as the mutation and serializability detection middleware.

[wldcordeiro]

I already added checks for mutation and serializability.

Regarding this @markerikson I find the sanity check for actions to be contrary to some other docs.

Note that it is okay to use Symbols, Promises, or other non-serializable values in an action if the action is intended for use by middleware. Actions only need to be serializable by the time they actually reach the store and are passed to the reducers.

https://github.com/reduxjs/redux/blob/master/docs/faq/Actions.md

It's a rather jarring console.error in my tests and browser and it'd be nice not to have to explain to each person why we have an error that's not one according to some of the docs.

[markerikson]

@wldcordeiro : part of the point of this package is to provide opinionated defaults on top of Redux. I added checking for serializability as part of that.

The serializability middleware is currently configured to run after redux-thunk, so it should only be seeing actions that have gone past that. If you've just called configureStore() without supplying any further middleware, then that's the last middleware in the chain, so it's reasonable to be checking at that point.

Can you provide some details on what you're putting in your actions, and how you're using configureStore()?

[wldcordeiro]

@markerikson

Here was the output from the middleware.

console.error node_modules/redux-starter-kit/dist/redux-starter-kit.cjs.js:116
      A non-serializable value was detected in an action, in the path: `errorHandler`. Value: { [Function: errorToastHandler]
        [length]: 1,
        [name]: 'errorToastHandler',
        [prototype]: errorToastHandler { [constructor]: [Circular] } }
      Take a look at the logic that dispatched this action:

const action = {
  type: 'breadcrumbs/FETCH_BREADCRUMBS_RE$ULT',
  subjectAction: 'breadcrumbs/FETCH_BREADCRUMBS',
  handlerOpts: {},
  success:
    { breadcrumbs:
      [ { objCode: 'PORT',
          objID: '56fef1190000005dd035f65817bfa65b',
          name: 'TestPortfolio' },
        { objCode: 'PRGM',
          objID: '5a5e678e0000117a78e08c5f81bd2c2d',
          name: 'TEST PROG' },
        { objCode: 'TASK',
          objID: '5b7f2e6a00000128d52cbf178e42728f',
          name: 'Task Nav1' },
        { objCode: 'TASK',
          objID: '5b7f2e8d0000017c205ca1b0b34c216c',
          name: 'Task nav sub1' },
        { objCode: 'TASK',
          objID: '5b7f2ea3000001a2e0e9696ffa1c1904',
          name: 'Task nav sub1-1' },
        { objCode: 'TASK',
          objID: '5b7f2eba000001c89c5707c5720067a0',
          name: 'Task nav sub1-1-1' },
        { objCode: 'TASK',
          objID: '5b7f2ecb000001ee47a50c5e8d37b121',
          name: 'Task nav sub 1-1-1-1' },
        { objCode: 'TASK',
          objID: '5b7f2ee800000214596e69d5d4585f5e',
          name: 'Task nav sub 1-1-1-1-1' },
        [length]: 8 ] },
  suppress: false,
  errorHandler:
    { [Function: errorToastHandler]
      [length]: 1,
      [name]: 'errorToastHandler',
      [prototype]: errorToastHandler { [constructor]: [Circular] } },
  successHandler: null,
};

I am using redux-observable in newer projects and have some older ones that use redux-thunk since the docs made it seem like the middleware array was empty save for redux-thunk (https://redux-starter-kit.js.org/api/getdefaultmiddleware#getdefaultmiddleware) I had just overridden the array to be the defaults + redux-observable with the intention of removing the defaults afterwards once I migrated my code.

In my usage of redux-observable I have made a success/error handler system with a similar utility to createAction called createResult where you can register an error or success handler function to be used by the result aggregator in the middleware.

https://gitlab.com/wldcordeiro/stuffz/blob/master/packages/redux-utils/src/results/create-result.js

[markerikson]

Yeah, I haven't had a chance to update the docs since I added the extra runtime check middlewares. Still on my todo list.

In general, I would say that the idea of passing a callback function to a middleware by putting it in an action is a legal thing to do with Redux. But, it's also not what I'd consider the "80%+ use case", which is what configureStore() and getDefaultMiddleware() are trying to solve.

Since you're already defining the list of middleware you want to pass it, it's probably best if you drop calling getDefaultMiddleware(), and explicitly define the complete list of middleware you want.

[wldcordeiro]

@markerikson I didn't know of the sanity checks being done in a middleware. 😄 Perhaps making that middleware configurable would be nice to have if it's an export. I still see value in warning on the state, just not for actions with the setup I'm using.

[markerikson]

Yeah, I just added those middlewares about a week ago.

The point of getDefaultMiddleware() is that it literally returns the default middleware :) If you prefer to customize the middleware setup, passing the middleware argument to configureStore() completely overrides the defaults, which is also why getDefaultMiddleware() is exported so that you can call it yourself if needed (like middleware : [...getDefaultMiddleware(), reduxObservable(rootEpic)]).

I'd be open to a PR that allows customizing the serializability check handling, with the caveats that A) we're close to merging in #73 which will rewrite everything in TS, and B) the actual default behavior would still be to check action contents out of the box. What I envision being configurable is:

import {createSerializableInvariantMiddleware, configureStore} from "redux-starter-kit";

const store = configureStore({
    reducer : rootReducer,
    middleware : [
        thunk,
        createSerializableInvariantMiddleware({actions : false}),
        reduxObservable(rootEpic)
    ]
});

[wldcordeiro]

@markerikson You've envisioned it like me too. I just was confused by the docs mentioning the default just being thunk and then suddenly seeing the console.error now that I've dug around it's pretty clear what all is being done and how I would use the middleware on my own.

[markerikson]

Yup. Sorry, updating the docs on this topic slipped through the cracks. I also really want to add a "Usage Guide" section and a "Tutorial" section, but need to figure out how to approach those.

If you've got a few minutes, think you could file a PR to update the getDefaultMiddleware() page with some notes on the runtime check middlewares?

[markerikson]

@wldcordeiro : I updated the docs tonight to reflect the current behavior:

https://redux-starter-kit.js.org/api/getdefaultmiddleware

Hopefully that helps :)

[markerikson]

The notion of checking for async behavior in reducers is an interesting one, but realistically the complexity and overhead isn't worth it. Closing this.