atlassian / react-sweet-state

Shared state management solution for React
https://atlassian.github.io/react-sweet-state/
MIT License
871 stars 55 forks source link

Accessing previous props in onUpdate? #131

Closed obweger closed 1 year ago

obweger commented 3 years ago

Hi team,

I occasionally find myself wanting to make more fine-grained decisions about what to do in onUpdate, requiring me to understand how exactly the props changed. However, as I only have access to the current props, I have to put the props into the state for later comparison. That seems not overly elegant to me. Would it be possible to make the previous props accessible in onUpdate?

albertogasparin commented 3 years ago

Is that because you might not want to trigger the update function at all or just diffing individual props? As we might expose an option to override the "shouldUpdate" function so you can bail out of updates. But not sure about providing prev props to update actions as would complicate things a bit and make the function signature different from all other actions 🤔

obweger commented 3 years ago

Sometimes so, sometimes so, but mostly diffing on individual props. Agree that providing prevProps would make the function signature different from the others, but I personally think that's okay. Just for reference (and obviously without any context), here's an example onUpdate from JXL:

    onUpdate: () => ({ getState, setState, dispatch }, props) => {
        const { props: prevProps } = getState();

        const { issueLoadingResult, editMode } = props;

        setState({ props: { issueLoadingResult, editMode } });

        if (prevProps.issueLoadingResult !== issueLoadingResult) {
            dispatch(initialize());
        } else if (prevProps.editMode !== editMode) {
            dispatch(resetQueue());
        }
    },
anacierdem commented 3 years ago

This is something that one must do frequently to be able to make the store communicate with the outside world. This is another common pattern we use a lot;

onUpdate: () => (
  { getState, setState },
  { activeId, activeQuery }
) => {
  const {
    activeId: existingActiveId,
    activeQuery: existingActiveQuery,
  } = getState();

  if (existingActiveId !== activeId) {
    setState({ activeId });
    dispatch(doSomethingWhenIdChanges);
  }

  if (existingActiveQuery !== activeQuery) {
    setState({ activeQuery });
  }
}

This similar kind of pattern is almost a best practice to keep the store consistent with the outside via the update method. Although I think this is an area that RSS might find a common solution/pattern/tool, I'm not sure the action creator params is the best place to do this. Notice the fact that we are actually updating the state with the prop information here, instead of keeping an additional props state and prevProps would be a highly opinionated solution here. It would allow us removing the getState call, which is not much of a problem considering the flexibility it provides. Still, we would need to destructure prevProps with not much gain in the end. In @obweger 's example as well, it would only move prevProps to the creator params, which is not a huge improvement still;

onUpdate: (prevProps) => ({ getState, setState, dispatch }, props) => {
  const { issueLoadingResult, editMode } = props;

  setState({ props: { issueLoadingResult, editMode } });

  if (prevProps.issueLoadingResult !== issueLoadingResult) {
      dispatch(initialize());
  } else if (prevProps.editMode !== editMode) {
      dispatch(resetQueue());
  }
}

Also, there are potential use cases where the consumer may want to use an already existing action with optional parameters in place of the update function and introducing pre-defined parameters there may prevent that. So it does not map "well" with the action creator parameters, at least in my opinion. Especially considering the little benefit.

Still, I really think the update/init actions (or rather making multiple stores communicate) is a pain point which may need further thought from an RSS perspective. Exposing the comparison function might be an thing indeed.

albertogasparin commented 1 year ago

I'm happy to say that from v2.7.0 prev/next props are available on the new onContainerUpdate store handler:

const Store = createStore({
  initialState,
  actions,
  containedBy: TodosContainer,
  handlers: {
    onInit: (nextProps, prevProps) => ...
  },
});