Consider additional runtime checks

[nickserv]

• Warn if data structures (like Sets and Promises) are used in reducers that aren't serializable. We could potentially look for toJSON methods on objects, but i'm not sure how consistent it is.
• Warn about multiple stores, or make it more difficult/impossible.
• Warn about using Redux or the starter kit directly when react-redux is being used.
• Warn about action names that are overly generic or imperative like SET_*.

[markerikson]

Somewhat related to this: we already try to put redux-thunk in the middleware list by default. What would happen if we were to add a freezing/mutation detection middleware like redux-immutable-state-invariant (or other similar options) to the list as well?

Relevant questions:

• Our createReducer() relies on immer, and that freezes state in dev. What happens if you double-freeze objects? Errors? Perf hit?
• What about non-plain-JS values, like a single number (counter example) or an Immutable.js Map or something?

[nickserv]

I only have basic experience with immer, but I thought that it would already prevent mutations by wrapping the producer in a proxy, in which case we wouldn't need to detect mutations unless immer was disabled.

[markerikson]

The proxy only applies for the draftState value supplied to the produce callback. However, Immer does freeze the return value of produce():

const newState = produce(originalState, (draftState) => {
    // draftState is wrapped in a proxy
});

// newState has been deep-frozen

The other aspect is whether a user might only be using createReducer for parts of their app, in which case there would be other parts they might be mutating accidentally still.

[nickserv]

Ah I see, so the middleware still processes the entire store. Would there be a way to blacklist immer reducers?

[nickserv]

Alternatively, could we wrap the root reducer in immer's auto freeze automatically?

[markerikson]

I was just thinking about this again.

I think the two things we might want to check for are serializability and accidental mutations.

Checking for those at runtime would be potentially expensive, but it does seem like the kind of thing that would be worth adding in dev only, given that this is intended to be a reasonably opinionated package.

On that note, I'm inclined to say we enforce POJO data. If you've chosen to use Immutable.js, you probably aren't worried about mutations anyway, and you could opt out of this by not using the default list of middleware with configureStore().

Some googling turns up https://stackoverflow.com/questions/30579940/reliable-way-to-check-if-objects-is-serializable-in-javascript .

Thoughts?

[jeffmwells]

I like the idea of keeping the checks only in development mode. Had a discussion with my team today and we all agreed we’d love checks for accidental mutations.

[markerikson]

Oh, these would absolutely only be in dev mode. Haven't measured anything, but definitely don't want the runtime overhead in prod.

[taras]

Accidental mutation 👍 Serializable - if there is a way for the object to opt-in to being serializable.

[markerikson]

Generally, the only things I can think of that would commonly go in Redux that aren't obviously POJOs are Immutable.js Maps and Lists. Those are sorta-mostly serializable, although it requires special handling from the Redux DevTools extension.

I did a poll a few months back asking how many people were using Immutable.js or anything other than POJOs in their store

https://twitter.com/acemarke/status/1036729164538281984

Leaving out the "See Results" answer, 82% of respondees are using POJOS, 15% are using Immutable.js.

I think it's reasonable to focus on POJOs by default.

I did glance briefly at the Immutable.js repo to see if there were any good ways to detect if an object is an instance of an Immutable.js Map/List. It looks like you'd explicitly have to import a method from the library itself, and I don't want to add a dependency on that (even in dev only).

[taras]

Why not use valueOf? It's simple. That's what it does.

[markerikson]

Because I don't know enough about it to understand what you're suggesting :)

[taras]

Oh ok, sorry. @cowboyd and I have been using it for so long, I almost assume that everyone uses it this way. My apologies 😃

Usually, in JavaScript object.valueOf() returns the actual object that it's called on, but the class can overwrite what valueOf does. For classes that are meant to be serializable, the class can provide a valueOf implementation that would return serializable value.

class Person {
  constructor(firstName, lastName) {
    this.firstName = firstName;
    this.lastName = lastName;
  }
}

let bart = new Person('Bart', 'Simpson');

bart.valueOf() // this is not what we want because this is a complex instance
// Person

class PersonOfValue extends Person {
  valueOf() {
    const { firstName, lastName } = this;
    return { firstName, lastName };
  }
}

let bartOfDistinction = new PersonOfValue('Bart', 'Simpson');

bartOfDistinction.valueOf(); // this is what we want
// Object

Here is a runkit example: https://runkit.com/taras/making-something-serializable

[markerikson]

I suppose it kinda depends on what level of "serializability" we would want to try to enforce.

Overall, the primary reasons for keeping things serializable are to allow time-travel debugging and state persistence to work right.

For the time-travel debugging use case, we can assume the user is using the Redux DevTools Extension. That does have special handling for Immutable.js objects (using remotedev-serialize), and also uses the jsan library for serializing things like dates, cyclical references, etc.

I suppose the options are something like:

• Only accept POJO data
• Accept POJOs, or values that have toJSON() defined
• Accept POJOs, or values where valueOf() returns something POJO-ish

[taras]

Accept POJOs, or values that have toJSON() defined

Is this an Immutable.js thing?

Here is an example of what isSerializable(value) might look

function valueOf(value) {
    return value && value.valueOf ? value.valueOf() : value;
}

function isSerializable(value) {
    switch (valueOf(value).constructor) {
    case String:
    case Boolean:
    case Number:
    case Object:
    case Array:
        return true;
    default:
        return false;    
    }
}

I updated the runkit and added a test to verify it's behaviour https://runkit.com/taras/making-something-serializable

[taras]

I suppose it kinda depends on what level of "serializability" we would want to try to enforce. Overall, the primary reasons for keeping things serializable are to allow time-travel debugging and state persistence to work right.

This is the level of serialization that we use this pattern for, although it'll be flexible enough to accommodate most serialization needs.

We use this pattern to serialize and deserialize very complex objects in Microstates.js

[taras]

A Microstates of any complexity will serialize to a POJO by calling .valueOf(). Microstates will deserialize complex data structures as long as you know it's value and root type.

[taras]

Sorry I was being assy, it was not intentional 😄

[markerikson]

toJSON() is apparently a JSON.stringify() thing, and I think Immutable.js happens to implement that function in its classes.

While I dislike basing things on implementation details of another library, the primary use case here is compatibility with the Redux DevTools extension. Look at the jsan lib, I don't see any mentions of valueOf anywhere, so I'm not sure how helpful focusing on valueOf would actually be.

Also, looks like redux-persist's default serialization is just JSON.stringify().

[taras]

toJSON is a little different. toJSON creates a new reference for the value on every invocation. valueOf would maintain a stable reference to the underlying value of the object. So, calling valueOf(object) === valueOf(object).

While I dislike basing things on implementation details of another library, the primary use case here is compatibility with the Redux DevTools extension.

Serialization for purpose of time travel debugging is one application, but reference stability has wide ranging applications beyond serialization.

Look at the jsan lib, I don't see any mentions of valueOf anywhere, so I'm not sure how helpful focusing on valueOf would actually be.

It's not commonly used, but many challenges are simplified considerably when you can assume stable references from valueOf. This conversation is possible because Redux is immutable by design. For mutable objects, referential transparency is not really useful, hence valueOf is not commonly used.

[markerikson]

The overall goal here is to ultimately print console warnings to let the user know they've put non-serializable values into the store state, so that they don't accidentally break things like time-travel debugging. In other words, the store itself won't be doing the serialization - we're just trying to do some runtime lint/validation-like behavior to help keep users from making mistakes.

[taras]

Yeah, I understand.

I commented because how you choose to check if something is serializable, will impact implementation of libraries that integrate with Redux. I wanted to add some information to the considerations to reduce the risk that this change will discourage people from using Redux in more flexible ways.

Also, I want to make sure that Microstates remains compatible redux-starter-kit after this change 😄

[markerikson]

Merged support for checking for mutations and serializability in #63 , so I'd say this has been covered.