Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessing previous props in onUpdate? #131

Closed
obweger opened this issue May 6, 2021 · 4 comments
Closed

Accessing previous props in onUpdate? #131

obweger opened this issue May 6, 2021 · 4 comments
Labels
enhancement New feature or request

Comments

@obweger
Copy link
Contributor

obweger commented May 6, 2021

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
Copy link
Collaborator

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 🤔

@albertogasparin albertogasparin added the enhancement New feature or request label May 10, 2021
@obweger
Copy link
Contributor Author

obweger commented May 10, 2021

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
Copy link

anacierdem commented Jul 2, 2021

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
Copy link
Collaborator

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) => ...
  },
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants