aribouius / jsonapi-react

A minimal JSON:API client and React hooks for fetching, updating, and caching remote data.
MIT License
149 stars 28 forks source link

Query cancel #4

Closed webdobe closed 4 years ago

webdobe commented 4 years ago

Not sure if this is possible but it would be cool if a query could be cancelled before the next query fires. Or have some way of having the client cancel the query. I am still looking through the src to see how to do it but not coming up with anything yet.

Maybe you have some feedback on this idea?

Thank you!

webdobe commented 4 years ago

This is bug level at this point. Because, if a request is going out and returning results. If the next call returns results before the previous due to query changes. You will get wrong results back because the data was updated with the first call. We need to either cancel the request when a new client request is going out or provide ability to cancel through the client. Preferably both. This really shows up when using async calls with slower servers.

webdobe commented 4 years ago

Created a first attempt at this feature. Let me know what you think. It does not deal with mutations yet. Wanted you to review to make sure I was on the right path.

Thank you!

webdobe commented 4 years ago

The idea is you can call: const client = useClient([resource, query]); client.cancel([resource, query]); That would instantly cancel your query. You need to pass into the cancel either the queryArgs of your previous request or if you don't pass any then it cancels all queries.

aribouius commented 4 years ago

Hey @webdobe thanks for bringing this idea up. The ability to cancel requests has been on my todo list for a while, I just haven't gotten around to it yet.

From my perspective I actually see two separate issues at play here.

1) Ensure that stale/invalid query results don't get distributed to active components (bug). 2) Add ability to manually cancel requests (feature).

While the two concerns overlap, I'd prefer to separate them into two separate issues as the changes required for cancelations are a bit more nuanced.

Specifically: 1) While this library defaults to using fetch, it technically allows the user to provide a custom implementation of it (e.g. a wrapper around axios), and thus I want to make sure that any solution can be tapped into by such use cases. 2) The AbortController is still experimental, and is not supported at all by IE, which is unfortunately still a concern for many of us. While I'm not opposed to using it, it needs to handle it not being available.

I'm happy to work on a patch for issue #1 (hopefully today). I'll also start looking into how cancellations can be supported in a fashion that is browser/library compatible.

webdobe commented 4 years ago

@aribouius Works for me. I kinda felt the same way. Just figured if I actually contributed a start we could get the ball rolling. My approach was quick and dirty as I am not super familiar with the entire library you created. Appreciate any work you do.

Thank you

aribouius commented 4 years ago

Hey @webdobe, can you provide a description of a scenario where a stale/invalid query got distributed to an active component (i.e. navigated to page A, requested stalled out, navigated to page B, ...etc)

I'm not having much success replicating the issue. In looking at how queries are processed, I'm actually not sure how it could happen.

When useQuery is called, it attempts to create a state object, that is unique to the request URL. If a state object with the same URL is found, it is used instead of creating a new one.

The state object has a promise attached to it when a request is mid-flight. Thus if a query is stalling out, and its calling component un-mounts, and then later re-mounts, it will actually just receive the original state object and promise, assuming the query was configured to be cached (if not, the state object gets garbage collected, and the next request receives a new state object)

Thus is seems unlikely that a component could receive stale data given the requests are batched by URL.

I did notice a small inefficiency in the garbage collection procedure which I patched up here, but it didn't appear that it would affect this particular issue.

webdobe commented 4 years ago

I get them a lot using material-ui autocomplete. You could easily reproduce if one field updates the filter of another before the query for default values or during search and the previous search hasn't successfully returned yet.

Here is my component. If you see anything wrong with it I would be happily be educated.

import React, { useState, useEffect, useRef } from 'react';
import Autocomplete from '@material-ui/lab/Autocomplete';
import TextField from "@material-ui/core/TextField";
import { useClient } from "jsonapi-react";
import PropTypes from "prop-types";
import { buildQuery } from "../../helpers";

function JsonApiAutocomplete({
   resource,
   selected,
   getSelected,
   values,
   include,
   fields,
   filter,
   page,
   sort,
   textFieldProps,
   inputProps,
   searchBy,
   dependent,
   requireSearch,
   ...props
}) {
  const isMountedRef = useRef(null);
  const initialQuery = buildQuery({include, filter, fields, page, sort});

  const [query, setQuery] = useState(initialQuery);
  const [search, setSearch] = useState('');
  const [option, setOption] = useState(selected ? selected : null);
  const [state, setState] = useState({
    loading: false,
    error: false,
    options: values ? values : [],
  });
  const client = useClient([resource, query]);

  // Requery and set options to empty so it shows we are loading new data.
  useEffect(() => {
    // When filters update we re-query
    isMountedRef.current = true;
    if (!requireSearch || (requireSearch && query.filter && query.filter.search)) {
      if (isMountedRef.current) {
        getOptions();
      }
    }
    return () => isMountedRef.current = false;
  }, [query]);

  // If we pass in values or update values we need to update the options.
  useEffect(() => {
    setState({
      ...state,
      ...{
        options: values ? values : []
      },
    });
    return () => {};
  }, [values]);

  // If selected options change we need to update the selection option.
  useEffect(() => {
    if (state.options && state.options.length && typeof getSelected === 'function') {
      setOption(getSelected(selected, state.options));
    } else {
      setOption(selected ? selected : null);
    }
    return () => {};
  }, [selected]);

  // If search takes place we need to update the query so we will re-query.
  useEffect(() => {
    const timer = setTimeout(() => {
      let newQuery = buildQuery({include, filter, fields, page, sort});
      let searchFilter = newQuery.filter ? newQuery.filter : {};
      searchFilter[searchBy] = search;
      newQuery.filter = Object.assign({}, searchFilter);
      setQuery(newQuery);
    }, 500);
    return () => clearTimeout(timer);
  }, [search]);

  // We only want to update if filter changes if this field depends on values changing in another.
  useEffect(() => {
    if (dependent) {
      let newQuery = buildQuery({include, filter, fields, page, sort});
      setQuery(newQuery);
    }
    return () => {};
  }, [filter]);

  const getOptions = async () => {
    setState({
      ...state,
      ...{
        loading: true,
        options: values ? values : []
      },
    });

    const { data, message } = await client.fetch([resource, query], {
      cacheTime: 5 * 60,
      staleTime: 60,
    });

    if (message) {
      setState({
        ...state,
        ...{
          loading: false,
          error: message
        },
      });
    }

    if (data) {
      setState({
        ...state,
        ...{
          loading: false,
          error: false,
          options: data
        },
      });

      if (data.length && typeof getSelected === 'function') {
        setOption(getSelected(selected, data));
      }
    }
  };

  const searchOptions = (value) => {
    setSearch(value);
  };

  let noOptionsText = 'No options';
  if (requireSearch && query.filter && query.filter.search) {
    noOptionsText = `No options for search ${query.filter.search}`;
  } else if (requireSearch && (!query.filter || !query.filter.search)) {
    noOptionsText = `Enter search option`;
  }

  return (
    <Autocomplete
      {...props}
      options={state.options}
      loading={state.loading}
      value={option}
      noOptionsText={noOptionsText}
      renderInput={params => {
        return (
          <TextField
            {...params}
            fullWidth
            helperText={state.error}
            error={state.error ? true : false}
            {...textFieldProps}
            InputLabelProps={{
              shrink: true,
            }}
            inputProps={{
              ...params.inputProps,
              ...inputProps
            }}
            onChange={(event) => searchOptions(event.target.value)}
          />
        );
      }}
    />
  );
}

JsonApiAutocomplete.propTypes = {
  id: PropTypes.string.isRequired,
  getOptionLabel: PropTypes.func.isRequired,
  getOptionSelected: PropTypes.func,
  onChange: PropTypes.func.isRequired,
  renderOption: PropTypes.func,
  textFieldProps: PropTypes.shape({
    label: PropTypes.string.isRequired,
    variant: PropTypes.string,
    size: PropTypes.string,
  }),
};

JsonApiAutocomplete.defaultProps = {
  searchBy: 'search',
};

export default JsonApiAutocomplete;
aribouius commented 4 years ago

Hey @webdobe, I finally got a chance to read through your code sample.

So you're fetching directly via the client instance, which means the issue isn't related to the useQuery hook as I assumed previously. Since you are updating your own state when the request promise is fulfilled, i'm inclined to think that the issue lies with your code implementation, and not this library.

I suspect the issue is that your code updates the result state, without checking whether the fulfilled promise was issued by the current query options. I.e. if a request promise gets fulfilled after the user enters a new character into the input, the result should likely be discarded.

This is certainly something that would be more easily handled by having the ability to cancel the request. I'm still working on implementing it, and will update you when it lands in master.

aribouius commented 4 years ago

@webdobe hey again, thanks for your patience on this issue.

Version 0.0.16 now supports the ability to abort API requests, when fetching via the client instance.

const client = useClient()
const request = client.fetch(...)
request.abort()

When aborted, the promise will resolve with an error object.

webdobe commented 4 years ago

@aribouius appreciate your work on this. I will check it out.

webdobe commented 3 years ago

@aribouius

I tried this out and first I get errors secondly I am not sure how practical it is to have it attached to the fetch. For example

const getOptions = async (isMountedRef) => {
    setState({
      ...state,
      ...{
        loading: true,
        options: []
      },
    });

    request = await client.fetch(...);

    // throws Uncaught (in promise) TypeError: request.abort is not a function
    // after it aborts I believe
    request.abort();

    const { data } = request;

    if (data) {
      setState({
        ...state,
        ...{
          loading: false,
          options: data
        },
      });
    }
  };

  useEffect(() => {
    // When filters update we re-query
    let timeout;
    isMountedRef.current = true;
    if (state.item && isMountedRef.current) {
      timeout = setTimeout(() => {
        getOptions(isMountedRef);
      }, 1000);
    }
    return () => {
      // I want to abort here...
      clearTimeout(timeout);
      isMountedRef.current = false;
    }
  }, **[state.item]);**

In the above example I would like to abort the request when the useEffect returns (unmounts). Basically a search field sets state.item when this changes I should be able to abort the previous request and determine if we need to create a new one.

maybe you could provide some insight to maybe something I am doing wrong?

Thank you! -Jesse