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

Query component does not update #3126

Closed
RafeSacks opened this issue Apr 15, 2019 · 6 comments · Fixed by #3146
Closed

Query component does not update #3126

RafeSacks opened this issue Apr 15, 2019 · 6 comments · Fixed by #3146
Assignees
Labels

Comments

@RafeSacks
Copy link

Background:
The simple user story for background here is that I wanted to select a customer from a list and then display a list of locations associated with the customer to pick from. The first reference (customer) is sorted out by a ReferenceInput which then triggers a FormDataConsumer which renders a custom component which uses a Query to look up the customer data and then pass the locations list of ids to a nested Query to get the locations for use in a SelectInput. (Let me know if it would be helpful to see the code implementing the pattern above.)

What you were expecting:
When changing customers I expected the locations SelectInput to be re-rendered with the new customer's locations as choices

What happened instead:
When setting a customer the first time it all worked fine. When changing the customer after that initial time all the machinery fired off but the data in the Query was from the initial customer.

Related code:
I fixed the issue by implementing componentDidUpdate. I copied the react-admin Query component and replaced componentDidMount with all of the code here:

  queryAndSetState = () => {
    const { dataProvider, type, resource, payload, options } = this.props;
    dataProvider(type, resource, payload, options)
      .then(({ data, total }) => {
        this.setState({
          data,
          total,
          loading: false
        });
      })
      .catch(error => {
        this.setState({
          error,
          loading: false
        });
      });
  };

  componentDidMount = () => this.queryAndSetState();

  componentDidUpdate = prevProps => {
    // This should be enough to ensure we update when we want and avoid an infinte loop
    //   JSON pattern should work here since we already rely on it for the
    //   simple types used for query objects. We could also compare using
    //   ids only but more parsing would be needed.
    if ( JSON.stringify(prevProps.payload) === JSON.stringify(this.props.payload) ) {
      return;
    }

    this.queryAndSetState(prevProps);
  };

Environment

  • React-admin version: ^2.8.5
  • React version: ^16.8.6
@Kmaschta
Copy link
Contributor

I agree, the <Query> component run a request at mount and never run any additional request, even in a case of a re-render / update / force update.

It would be great to provide a way to fetch the query again, either by re-render the component or with a children prop.

@RafeSacks
Copy link
Author

RafeSacks commented Apr 15, 2019

It would be great to provide a way to fetch the query again, either by re-render the component

The code I posted fixes the issue (assuming someone with more experience reviews it and agrees on the details). componentDidUpdate only runs after the component is re-rendered. Basically the loop is:

  1. Component is re-rendered by React.
  2. componentDidUpdate runs and finds the payload has changed, state is updated.
  3. setState causes React re-render the component which reruns client-code children with updated data
  4. componentDidUpdate runs again but exits due to the payload test (blocks infinite loop)

...Based on what I've read this is how React is supposed to do it (render 2 times) and componentDidUpdate is the recommended place to query external sources and update state.

@Kmaschta
Copy link
Contributor

Thank you for the proposal, please open a PR so we'll be able to discuss about the code on it!

@RafeSacks
Copy link
Author

Did you see my reply at the Merged link? I found an issue that will be considered a bug when someone else hits it.

@fzaninotto
Copy link
Member

I have read it but not understood it. If there is a bug, please open a GitHub issue using the issue template, and allowing us to reproduce the bug.

@RafeSacks
Copy link
Author

It isn't so much a bug as a pattern that needs to be included. Since querying is asynchronous it is possible that a component will be unmounted before control returns, creating a race that will cause an error. Check out the trashable react, a package that solves, and explains the core issue. I've used this in my implementation and it did the trick:
Explained: https://github.com/hjylewis/trashable#trashable-put_litter_in_its_place
HOC Package: https://www.npmjs.com/package/trashable-react

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

Successfully merging a pull request may close this issue.

3 participants