grammarly / focal

Program user interfaces the FRP way.
Apache License 2.0
723 stars 34 forks source link

Set state in state.subscribe() #21

Closed sladiri closed 6 years ago

sladiri commented 6 years ago

Hi, I find focal very interesting, since it tries to use both imperative and functional approaches.

I played around and wanted to implement a pattern, where a click merely proposes new state value. Then a central function decides if the state has to be modified. It seems to work surprisingly, if I directly mutate the state in a state-subscription.

Do you think this is very wrong and that it is a "loophole" in the API that might not work correctly? I just tried the example from your Readme, where I added a Toggle component.

(Btw. I do not use Typescript)

import * as React from "react";
import { Atom, F } from "@grammarly/focal";

export default ({ shim }) => {

  const Counter = ({ count, onClick }) => (
    <F.div>
      You have clicked this button {count} time(s).
      <button onClick={onClick}>Click again?</button>
    </F.div>
  );

  const Toggle = ({ value, onClick }) => (
    <F.div>
      clickValue: {value}&nbsp;
      <button onClick={onClick}>toggle</button>
    </F.div>
  );

  const App = ({ state }) => (
    <div>
      Hello, world!
      <Counter
        count={state.view(x => x.count)}
        onClick={() => state.lens(x => x.count).modify(x => x + 1)}
      />
      <Toggle
        value={state.view(x => (x.toggle.value ? "Foo" : "Bar"))}
        onClick={() =>
          state.lens(x => x.proposal).modify(x => ({ toggle: !x.toggle }))}
      />
    </div>
  );

  const state = Atom.create({
    count: 0,
    toggle: { value: true },
    proposal: {}
  });

  state.subscribe(x => {
    console.log(`New app state: ${JSON.stringify(x)}`);
  });

  state
    .skip(1)
    .distinct(x => x.proposal)
    .do(x => {
      console.log("Proposal changed", x.proposal);
      if (x.proposal.toggle !== undefined) {
        x.toggle.value = !x.toggle.value; // This seems to actually work.
      }
    })
    .subscribe();

  return <App state={state} />;
};
sladiri commented 6 years ago

There is already one problem I get, when I want to do something async in my subscription. The component which uses state.toggle.value does not update anymore.

  const asyncTest = val =>
    new Promise(resolve => {
      setTimeout(() => resolve(val), 1000);
    });

  state
    .skip(1)
    .distinct(x => x.proposal)
    .flatMap(x => asyncTest(x)) // Adding this line prevents update of UI.
    .do(x => {
      console.log("Proposal changed", x.proposal);
      if (x.proposal.toggle !== undefined) {
        x.toggle.value = !x.toggle.value;
      }
    })
    .subscribe();

Is it somehow possible to still see the component update here?

blacktaxi commented 6 years ago

Hi @sladiri, thanks for submitting this curious case.

TL;DR: it is not supposed to work and worked in your first example only by coincidence

Why does it work

The fact that it works in this case I think is more of a coincidence. I haven't ran your example, but you're saying that mutating the value of state.proposal.toggle in a callback of .do() actually makes the app update its visual state.

  1. So, one seemingly unexpected thing is that the application renders, updating its visual representation. This is not supposed to happen unless the state atom observable emits a new value. In Focal, a component is rendered when its props are changed or when any of its observable props emit a new value. Neither of that is happening then you do x.toggle.value = !x.toggle.value.

The thing is, you're doing your mutation at the same time that the state atom is updating, and you happen to be able to finish doing it before the component tree actually starts rendering. I haven't really looked into it, but I'm guessing that since .do()s default scheduler is immediate, and Focal components subscribe to app state only after they are mounted, your callback to .do() is executed before Focal's callbacks.

Try adding an .observeOn(Rx.Scheduler.async) or a .delay(1) before the call to .do() and see if it still works? I'm guessing that it will not!

  1. Another possibly unexpected thing is that the Toggle component renders. You would think that since the Toggle component depends only on the state.proposal.toggle value it should not be rendering when anything else is changed. Remember that in my previous point I mentioned that the application renders. In Focal, only components that have their dependent state changed are rendered.

But in your case, the Toggle component depends on the complete app state, since this is your expression for the value attribute:

value={state.view(x => (x.toggle.value ? "Foo" : "Bar"))}

See, you're .view()ing the state, not state.lens('proposal').lens('toggle'). Focal can't have any idea that you're only using the toggle value from that state object so it can not optimize to render Toggle only when the toggle flag is changed. So the Toggle component is rendered every time anything in the app state is changed.

Is it supposed to work?

From the previous section you already can see that it is not supposed to work. But adding to that, the bigger issue with this example is that by mutating the state value object you are breaking composability.

You should never mutate state values directly.

Even if such a mutation is able to propagate to the visual representation by some sort of coincidence, it definitely will not work in a general case and has a high change of introducing unexpected and extremely difficult to track bugs.

  1. State value mutation is not guaranteed to propagate to the next state value. This means that in a world where state values are constructed in a fashion of immutable data structure, your local mutation can be overwritten the next time application state changes. There is no guarantee that the local mutation will persist, and I think it actually would be nice to have a guarantee that it will not.

  2. State value mutation is not guaranteed to propagate to other viewers of the state. If your local state is just a part of a bigger app state tree obtained from a .lens() call, or if some parts of your app can view the app state through different lenses (for example state.lens('proposal').lens('toggle')), they are not guaranteed to receive your local state value modifications.

  3. State value mutation does not trigger state update notifications. The .subscribe() callbacks are simply not called when you change the state value locally. This makes the state atom mostly useless with respect to your state mutations since nobody else can observe them.

Let me know if you have any questions.

sladiri commented 6 years ago

Thank you for the comprehensive answer, I will have to go through it. I just wanted to point out that the update does work if I use the atom API (I tried it first, but I must have made a mistake somewhere). I also updated the lens state.view(x => (x.toggle.value ? "Foo" : "Bar")) to state.lens(x => x.toggle.value).view(x => (x ? "Foo" : "Bar")) now. :)

  state
    .skip(1)
    .distinct(x => x.proposal)
    .flatMap(x => asyncTest(x))
    .do(x => {
      console.log("Proposal changed", x.proposal, x.toggle.value);
      if (x.proposal.toggle !== undefined) {
        state.lens(x => x.toggle.value).set(!x.toggle.value); // Use lens here now
      }
    })
    .subscribe();
sladiri commented 6 years ago

You were correct, a delay(1) breaks my initial example. 👍

However, the lens API (state...lens...set) does not work if there is no delay (Promise) in my subscription. That is why I did not use it in that example, but tried to mutate state directly as a last resort. I did not expect it to work.

If I do use a delay, then the lens API seems to work fine, and this is actually my default, since I want to allow some async work in the chain.

Although that one component was depending on the whole state with state.view, the React dev-tools showed that only the relevant components rerendered. I am no expert in this regard, so this is my only test to see which components update.

blacktaxi commented 6 years ago

I'm not 100% certain what are you trying to achieve, sorry for that. But if I understand correctly, you want to change app state depending on some external condition, and trigger this with a click of a button.

The easiest way to achieve that is to bypass writing the proposed value to state, since it is not necessary in this situation:

const App = ({ state, onProposal }) => {
  const toggleValue = state.lens(x => x.toggle.value)

  return <div>
    Hello, world!
    <Counter
      count={state.view(x => x.count)}
      onClick={() => state.lens(x => x.count).modify(x => x + 1)}
    />
    <Toggle
      value={toggleValue.view(x => x"Foo" : "Bar")}
      onClick={() => onProposal(!toggleValue.get())}
    />
  </div>;
}

return <App
  state={state}
  onProposal={p => {
    setTimeout(() => {
      if (p !== undefined) {
        state.lens(x => x.toggle.value).set(p)
      }
    }, Math.random() * 1000);
  }}
/>;

Again, sorry if I have misunderstood the problem you're trying to solve, but maybe this example will give you a helpful pointer?

Regarding your initial implementation, I don't have an immediate suggestion on how to it, but if you want to work some more on that, I can suggest to try using .modify instead of .set. However I'm not sure it's going to be of any help. FWIW, I think you're taking a detour route with your approach since it seems to be overcomplicated, but I may not have the complete understanding of the problem.

sladiri commented 6 years ago

Thank you for your help. I thought it could be benefitial if everything was in the state (maybe visualising all the streams). It is much simpler without the added state and actually closer to what I had in mind in the first place. :)