4
\$\begingroup\$

This piece of code is aimed to cancel any ongoing search requests sent from the web application if next request is started, e.g. after user updates the search term.

I tested it in the final application and everything looks fine. It has already been deployed to the remote environment in our organization.

It appears to work fine, but are there any pitfalls in the code?

const ongoingRequests = [];
const fetchData = (text, activePage, itemsPerPage) => async dispatch => {
dispatch({ type: FETCH_ENTITIES });
const page = activePage - 1;

let response;

// cancel any ongoing search requests
if (ongoingRequests.length > 0) {
    ongoingRequests.map(x => x.cancel('next request started'));
    ongoingRequests.length = 0;
}

// start a new request
response = await (async () => {
    const cts = axios.CancelToken.source();
    ongoingRequests.push(cts);

    let path =
    text !== ''
        ? `/Search?PageNumber=${page}&PageSize=${itemsPerPage}&SearchTerm=${text}`
        : `?QueryLead=true&PageNumber=${page}&PageSize=${itemsPerPage}`;

    return axios
    .get(`${env_urls.api.entity}Entity${path}`, {
        cancelToken: cts.token,
    })
    .then(response => {
        return response;
    })
    .catch(error => {
        if (
        typeof cts !== 'undefined' &&
        typeof cts.token !== 'undefined' &&
        typeof cts.token.reason !== 'undefined' &&
        cts.token.reason.message === 'next request started'
        ) {
            // request cancelled, everything ok
        } else {
        throw error;
        }
    });
})();

// response is undefined if the request has been cancelled
if (typeof response !== 'undefined') {
    const data = response.data;

    dispatch({ type: FETCH_ENTITIES_SUCCESS, data });
}
};
\$\endgroup\$
  • 1
    \$\begingroup\$ "Does it work as intended?" : it's up to you to tell us :] \$\endgroup\$ – Calak Nov 23 '18 at 10:46
  • \$\begingroup\$ @Calak Everything is fine when I test it in the application but maybe there is an issue that I cannot see. Like some js construct that makes no sense ;) \$\endgroup\$ – michal Nov 23 '18 at 10:49
  • \$\begingroup\$ "Does it work as intended?" Did you test it? One of the prerequisites of posting on Code Review is that the code has to work to the best of your knowledge. Does it? You say it works in the application. Is that the final application that needs it? A test application? \$\endgroup\$ – Mast Nov 23 '18 at 10:55
  • 1
    \$\begingroup\$ @Mast I have updated the question. \$\endgroup\$ – michal Nov 23 '18 at 10:58
  • 1
    \$\begingroup\$ When I implement this type of search functionality I usually cancel the current request when the user presses a key (on the keypress, paste events) and start a timer for a fraction of a second. I only start the request when the timer expires. That way I don't start requests while the user is still typing. I've also rarely found it unnecessary to keep an array of requests. I just keep the currently executing one if any. \$\endgroup\$ – Marc Rohloff Nov 23 '18 at 17:44
1
\$\begingroup\$

You can rewritte this piece of code:

.catch(error => {
    if (
    cts !== 'undefined' &&
    cts.token !== 'undefined' &&
    cts.token.reason !== 'undefined' &&
    cts.token.reason.message === 'next request started'
    ) {
    } else {
    throw error;
    }
}

To:

.catch(error => {
    if (
        typeof cts === 'undefined' ||
        typeof cts.token === 'undefined' ||
        typeof cts.token.reason === 'undefined' ||
        cts.token.reason.message !== 'next request started'
    ) {
        throw error;
    }
}

And you can go further into the simplification using this trick.

\$\endgroup\$
  • 1
    \$\begingroup\$ imo, you shouldn't use typeof you should just write it as if (cts === undefined) || ... \$\endgroup\$ – Marc Rohloff Nov 23 '18 at 17:39
  • \$\begingroup\$ @MarcRohloff yeah, copy/paste's mistake :p Thx \$\endgroup\$ – Calak Nov 23 '18 at 17:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.