2

I currently have a list of coordinates, which i use to fetch nearby hikes. Lets say I update my list state to new coordinates, but the fetch requests for the previous coordinates were not complete-- How can i cancel all the fetch requests made for the previous coordinates and only call new fetch requests based on the new coordinates?

I tried implementing AbortController but its probably not working as it's not cancelling a "batch" of fetch requests, but only single fetch request. My code is below:

  let abortController = new AbortController();
  const getNearbyHikes = async (coordinates) => {
    abortController.abort();
    abortController = new AbortController();
    for (const coord of coordinates) {
      if (coord) {
        try {
          const { lat, lng } = cord.coordinates
          const response = await fetch(
            'http://localhost:5000/category/' + lat + "/" + lng + '/' + searchCategory,
            {signal: abortController.signal}
          )
          const result = await response.json();
          setHikes((prevState) => [...prevState, ...result.businesses])
        } catch (error) {
          if(error.name === 'AbortError'){
            return;
          }
          throw error
        }

      }
    }
  }

Also , I want to update my UI with the hikes as I get them so it doesnt seem like my web app is really slow so I avoid using Promise.all. Any suggestions or help is greatly appreciated.

benwl
  • 366
  • 2
  • 17
  • 2
    `const response = await` your waiting, not batching so there will only ever be one to cancel. Have a look at using `Promise.all` to batch – Keith May 30 '22 at 08:40
  • This looks like it's basically a typo. You're providing the signal as the second argument, but that's not what `fetch` expects, it expects an **object** with a `signal` property (and optionally others). See: https://developer.mozilla.org/en-US/docs/Web/API/fetch#syntax – T.J. Crowder May 30 '22 at 08:40
  • @Keith Yea thats what i assumed, but i really want to avoid using `Promise.all` as I dont want my user to wait for all the data to load before its displayed. Would `Promise.all` still be a good solution or is there another suggestion. Thank you!! – benwl May 30 '22 at 08:46
  • @T.J.Crowder Thank you, I didnt catch that although it doesnt change my outcome. – benwl May 30 '22 at 08:47
  • @benwl - That suggests a problem in code that isn't shown, since that will work. – T.J. Crowder May 30 '22 at 08:50
  • @benwl Yes, if using `Promise.all` your still able to update your display. You basically attach to the `thenable`, .. It's kind of the technique I used for creating a `Promise.all` with a progress callback -> https://stackoverflow.com/questions/42341331/es6-promise-all-progress – Keith May 30 '22 at 13:54

1 Answers1

5

The main problem with the cancellation is that you're providing the signal to fetch incorrectly. You're providing the signal as a second argument, but fetch expects an "init" object there with a signal property on it.

As @Keith points out, though, your current code is making one call at a time. That will work, but you probably would benefit from doing the calls in parallel.

Something like this:

const getOneResult = async ({ lat, lng }, signal) => {
    try {
        const response = await fetch(
            "http://localhost:5000/category/" +
                lat +
                "/" +
                lng +
                "/" +
                searchCategory, // *** Where does this come from?
            { signal }
        );
        if (signal && signal.aborted) {
            throw new DOMException("Request cancelled", "AbortError");
        }
        if (!response.ok) {
            throw new Error(`HTTP error ${response.status}`);
        }
        return response.json();
    } catch (error) {
        if (error.name === "AbortError") {
            return null;
        }
    }
};
const getNearbyHikes = async (coordinates, signal) => {
    controller.abort();
    controller = new AbortController();
    const results = await Promise.all(
        coordinates.map((coord) => getOneResult(coord.coordinates, signal))
    );
    if (signal && signal.aborted) {
        return;
    }
    const businesses = [];
    for (const result of results) {
        businesses.push(...result.businesses);
    }
    setHikes((prevState) => [...prevState, ...businesses]);
};

Finally, I would have the caller supply the signal rather than using a global one for all calls to getHikes:

const getOneResult = async ({ lat, lng }, signal) => {
    try {
        const response = await fetch(
            "http://localhost:5000/category/" +
                lat +
                "/" +
                lng +
                "/" +
                searchCategory, // *** Where does this come from?
            { signal }
        );
        if (signal && signal.aborted) {
            throw new DOMException("Request cancelled", "AbortError");
        }
        if (!response.ok) {
            throw new Error(`HTTP error ${response.status}`);
        }
        return response.json();
    } catch (error) {
        if (error.name === "AbortError") {
            return null;
        }
    }
};
let controller = new AbortController();
const getNearbyHikes = async (coordinates) => {
    controller.abort();
    controller = new AbortController();
    const results = await Promise.all(
        coordinates.map((coord) => getOneResult(coord.coordinates, signal))
    );
    if (signal && signal.aborted) {
        return;
    }
    const businesses = [];
    for (const result of results) {
        businesses.push(...result.businesses);
    }
    setHikes((prevState) => [...prevState, ...businesses]);
};

then the caller controls aborting the request. But the former may be fine for your use case.


Side note: You'll see I've added a check for response.ok in there. Your code was assuming that since the fetch promise was fulfilled, the HTTP call worked, but unfortunately that's a footgun in the fetch API: it only rejects its promise on network failure, not HTTP failure. You have to check for the latter explicitly.

T.J. Crowder
  • 1,031,962
  • 187
  • 1,923
  • 1,875
  • Gah, I accidentally dropped your handling of `AbortError`, **and** you probably need at least one explicit check on `signal.aborted` (in case the `fetch` was already complete). – T.J. Crowder May 30 '22 at 08:54
  • Thank you for the thorough explanations and suggestions. Im pretty new and i just need some time to digest what you said. But to clarify something simpler for now, how come i should do `...results.map(({businesses}) => businesses)` instead and could you explain for about what you mean about further processing? – benwl May 30 '22 at 09:03
  • @benwl - That part of the answer was basically wrong I think, your original code expected an array of businesses on each result and pushed them all to `setHikes` individually, but that code would have tried to pass an array of arrays. I've updated the answer to fix the issues I mentioned above and to also handle `businesses` correctly (I think). – T.J. Crowder May 30 '22 at 09:05