rickyvetter / reductive

Redux in Reason
MIT License
409 stars 40 forks source link

Component doesn't update when props changed #40

Closed rusty-key closed 5 years ago

rusty-key commented 5 years ago

I have a component that is connected to the global state and also has prop passed from parent component:

let s = ReasonReact.string;
let component = ReasonReact.statelessComponent(__MODULE__);

let cmake = (~state: Storage.state, ~dispatch as _, ~counter, _children) => {
  ...component,
  render: _self =>
    <div className="counter">
      {counter->string_of_int->s}
      " / "->s
      {state.limit->string_of_int->s}
      " words left"->s
    </div>,
};

let make = (~counter, _children) => {
  ...component,
  render: _self => <Storage component={cmake(~counter)} />,
};

When the counter is updated, the component doesn't re-render.

After looking at source code, I found out that component gets updated only when the global state is changed: https://github.com/reasonml-community/reductive/blob/4e7ecfad749032079a64af4cbefca3f7b59c5ff5/src/reductive.re#L100-L101

@rickyvetter, @nlfiedler, I understand that it is done to avoid unnecessary re-renders, but what else can trigger re-render? Is this check really necessary? If it is, what can be done to allow re-renders on props update?

ambientlight commented 5 years ago

@rusty-key: how is counter updated exactly?

rusty-key commented 5 years ago

@ambientlight, does it matter? It is passed from the parent component. Here's an example:

type action =
  | IncrementCounter;

type state = {counter: int};

let component = ReasonReact.reducerComponent(__MODULE__);

let make = _children => {
  ...component,
  initialState: () => {counter: 0},
  reducer: (action, state) =>
    switch (action) {
    | IncrementCounter => RR.Update({counter: state.counter + 1})
    },
  render: self => {
    let {counter} = self.state;
    <div>
      <button onClick={_ => self.send(IncrementCounter)}>
        "Increment"->ReasonReact.string
      </button>
      <Counter counter />
    </div>;
  },
};
rickyvetter commented 5 years ago

I think it should also check to see whether the component prop has changed using retainedProps. That would definitely allow this kind of currying, but there might be other edge cases. At first glance I think that would cover all cases barring mutation and even then I think you're pretty limited in mutation because component is a function. You'd have to drop into JS to mutate the args list or something, so I think that's satisfactory.

Are you interested in putting up a PR for this?

ambientlight commented 5 years ago

shouldUpdate approach to avoid re-renders needs to be reconsidered since we don't know if child component got updated.

but what else can trigger re-render? Is this check really necessary?

The main motivation is avoiding needless rerenders when substates update. Example can be:

module CounterProvider = {
  let make = Reductive.Lense.createMake(~lense=store => store.counter, store);
};
module StringProvider = {
  let make = Reductive.Lense.createMake(~lense=store => store.content, store);
};
type appState = {
  counter: int,
  content: string,
};

Here is were was the problem, Lense component updates its state on each action dispatched to the store, which will cause re-render even when the substate of interest is not updated:

https://github.com/reasonml-community/reductive/blob/4e7ecfad749032079a64af4cbefca3f7b59c5ff5/src/reductive.re#L88-L92

If we actually perform the shallow comparison of lensed state inside the reducer, that will not require doing it in shouldUpdate, I just verified this approach to be working

rickyvetter commented 5 years ago

shouldUpdate approach to avoid re-renders needs to be reconsidered since we don't know if child component got updated.

I think this is a pretty aggressive stance. If the child component's state updates then it will still re-render. If it's props change (Reductive state), it will rerender. It's just this case of currying additional props that is problematic which is solved by checking component.

Not doing an update on state that is equal and removing shouldUpdate will also fix the problem though, so I'm open to either approach.

ambientlight commented 5 years ago

@rickyvetter: typo in my formulation, I missed props in there, but I implied that additional props actually

see whether the component prop has changed using retainedProps

Is the any way to access the retained props of child component?

I am submitting a small PR then.

rickyvetter commented 5 years ago

Is the any way to access the retained props of child component?

No you can't tell this - but in this case all you would need to do would be tell if your own component prop changed - since identity would be tied to values passed to it.