Allow callback style setState

[StevenLangbroek]

This PR allows the "callback" style setState, which React also allows. Where React allows it for asynchronicity, the change here would allow advanced users who are aware of (im)mutability concerns (which you take care of with Object.assign, to a certain extent) to make all state changes with a single pass of lodash.flow or R.evolve:

import { flow, update, set, add } from 'lodash/fp';

class MyState extends Container {
  increment() {
    this.setState(flow(
      update(['nested', 'counter'], add(1)),
      set(['didIncrement'], true)
    ));
  }
}

"Advanced" (imagine airquotes, thanks :) ).

import { flow, update, set, add, subtract, multiply } from 'lodash/fp';

class MyState extends Container {
  constructor() {
    super();

    this.increment = this.createSetState(add, 1);
    this.decrement = this.createSetState(substract, 1);
    this.double = this.createSetState(multiply, 2);
  }

  createSetState(method, count) {
    return () => this.setState(flow(
      update(['nested', 'counter'], method(count))
    ));
  }
}

Todo

• decide on Object.assign when using callback style
• add tests
• write documentation
• bump versions, publish etc.

[StevenLangbroek]

Want me to write some basic tests?

[jamiebuilds]

Shouldn't this still be merged back into the existing state?

[StevenLangbroek]

That's a good point. In the example it would just allocate an unnecessary empty object and key-iteration round (with Object.assign), as lodash/fp / ramda take care of immutability.

[jgoux]

Hello, I'd prefer it to stay that way so it reflects React's default behaviour.
Thanks again @jamiebuilds for this excellent project.

EDIT: Today I learned that the callback setState is merged back to the state! 💡

[StevenLangbroek]

How about this?

1. Run updater
2. Check for null / undefined or identity equal to previous state
3. If not, assign over defensive copy

step 2 would mean that if you bail early in your updater that decision is propagated to your components. @jamiebuilds?

[jamiebuilds]

Sure, sorry there aren't any already. I wanted to publish it before I stayed up to 2am

[StevenLangbroek]

Haha fair enough. Will add tests + when we iron out the above comment appropriate documentation.

[StevenLangbroek]

Is this even necessary? Or should I just return?

[Andarist]

assignment indicates that it changes something, I think return would be more appropriate here

[benwiley4000]

I think returning early if a shallow comparison fails would be a breaking change, and incompatible with React's setState behavior. React docs discourage returning mutated state from the setState updater function (or passing a mutated state object to setState), but neither is prohibited. From the React docs:

setState() will always lead to a re-render unless shouldComponentUpdate() returns false.

An example demonstrating a successful state-mutation update is here.

My second concern is that React emits its own development-time console warnings for passing null to setState, and by returning silently we might prevent a developer from realizing they have made a mistake.

For both of those reasons, I think it might be best to eliminate the conditional early return altogether, and let React do its thing.

[benwiley4000]

So in other words, I would think about rolling back ff77c96.

[Andarist]

setState() will always lead to a re-render unless shouldComponentUpdate() returns false.

I think this is not true with react@16. I don't use this feature but returning null is supposed to prevent scheduling a re-render.

What we might need to introduce here is 2 distinct paths for objects & function styles. Because of the null issue we cannot combine both and use newState in following lines, it should be restructured to something like:

  setState(state: $Shape<State> | (State => $Shape<State>)): void {
    const prevState = this.state;
    let newState
    if (typeof state === 'function') {
      newState = state(prevState)
      
      if (newState === null) {
        return;
      }
    } else {
      newState = state

      if (process.env.NODE_ENV !== 'production' && newState === null) {
        console.warn('write a nice dev warning here')
      }
    }

    if (newState === prevState) {
      return
    }

    this.state = Object.assign({}, prevState, newState);
    this._listeners.forEach(fn => fn());
  }

Does it make sense to u?

[StevenLangbroek]

Yeah I understand how to get there code-wise, I'd just like to have @jamiebuilds's input on what the desired behavior is.

[Andarist]

Well, the API should mirror React's API as closely as it can. So I'd say we should just follow that path.

[StevenLangbroek]

Yeah since this is already only conceptually similar to setState, I'm not convinced we should be following React to the letter vs doing the principally right thing. Their API tradeoffs aren't necessarily the same as those of a pure state manager. I'll update tomorrow, and hopefully get some time in to write those tests.

[jamiebuilds]

I want it to match the React behaviour as closely as possible. Refactoring component state into container state should be near seamless

[benwiley4000]

Wow my mistake! I was operating under the impression the Subscribe component's onUpdate() subscription callback was receiving the updated state and passing it to setState(). I should have read more closely. 🙂

@StevenLangbroek Given the above discussion I concede it makes sense to have custom error reporting and perhaps disregard what React does or does not report as a mistake.

However, the documentation for Unstated is currently written without any specification for setState(). The implication is that setState should behave the same way you would expect already, having worked with React. That means:

• null prevents a re-render
• But equal object identity does not prevent a re-render.

If there's an intent to change that, it should be documented well, and also be considered a breaking change which would merit a new major version.

[benwiley4000]

@jamiebuilds I missed your reply, but I guess I should have just let your comment sit. 😄 I appreciate that sentiment - it's my favorite thing about this library.

[gajjardarshithasmukhbhai]

  if (process.env.NODE_ENV !== 'production' && newState === null) {
        console.warn('write a nice dev warning here')
      }

Hi @StevenLangbroek , can we make one constant file and manage this kind of string production ? because in code we are using this kind of thing repeatedly and so instead of doing repetitively we can handle it in one file use.

[bnolan]

Any progress on a fix for this?

[gajjardarshithasmukhbhai]

  if (process.env.NODE_ENV !== 'production' && newState === null) {
        console.warn('write a nice dev warning here')
      }

Hi @StevenLangbroek , can we make one constant file and manage this kind of string production ? because in code we are using this kind of thing repeatedly and so instead of doing repetitively we can handle it in one file use.

[gajjardarshithasmukhbhai]

instead of "function", can we make it in a constant file and export it @StevenLangbroek