How to handle this fetch() TypeError?

Question

I am trying to print to the console (in order to later handle those edge cases) the response status of my fetch query. However, the only console.log call that works is the one in the 'breaches' function. I get no errors when an account exists in the HIBP db, but I get a 'Request failed: TypeError: response.json is not a function at json' error when the account is not in the db. What am I doing wrong? I got the error handling code from the Google Web Dev articles.

function createNode(element) {
    return document.createElement(element);
}

function append(parent, el) {
    return parent.appendChild(el);
}

function status(response) {
    if (response.status >= 200 && response.status < 300) {
        return Promise.resolve(response)
        console.log('all is good');
    } else if (response.status == 404) {
        return Promise.resolve(response.statusText)
        console.log('no breaches');
    } else if (response.status == 400) {
        return Promise.resolve(response.statusText)
        console.log('bad req');
    } else {
        return Promise.reject(new Error(response.statusText))
    }
}

function json(response) {
    return response.json()
}

var account = document.getElementById('account'),
    results = document.getElementById('results');
account.addEventListener("keyup", keyupEvent);

function keyupEvent() {
    event.preventDefault();
    if (event.key === "Enter") {
        fetch('https://haveibeenpwned.com/api/v2/breachedaccount/' + account.value, {
                timeout: 1500,
                userAgent: 'test'
            })
            .then(status)
            .then(json)
            .then(function(breaches) {
                console.log('Status Code: ' + breaches.status);
                let span = createNode('span');
                return breaches.forEach(function(check) {
                    span.innerHTML = `${check.Name}<br/>`;
                    append(results, span)
                })

            }).catch(function(error) {
                console.log('Request failed:', error);
            });
    }
}

Side note: if (response.status >= 200 && response.status < 300) { => if (response.ok) — T.J. Crowder
Your console.logs in status will never be executed, as they're after the return in each branch. — T.J. Crowder
Also your status function has no need to create new promises, just have it directly (and synchronously) return the desired result, or throw for the final else clause. Creating additional promises like this is a common promise anti-pattern. — Alnitak
Thanks, @T.J.Crowder and Alnitak. I don't really understand Promises - I've read an article about them - so I don't really know how to use them. As for the return fiasco, yeah, that was... embarassing :) — user10482604

T.J. Crowder T.J. Crowder · Accepted Answer · 2018-10-11T10:06:15

Your status function returns (a promise of) the status text for responses that are 400s or 404s. Your promise chain consuming the fetch result doesn't handle that possibility; it assumes it gets the response object.

You probably want to reject on 400s or 404s rather than resolving, but if not, you'll need to branch in your then handler expecting to read JSON.

Your code consuming the breaches is also overwriting the same span and repeatedly appending it; it will end up appended only once, with the last breach's information. And the append function doesn't provide any useful abstraction over just calling appendChild.

If the API really returns 404 for "no breaches" (blech), then I'd get rid of createNode and append, change status to this:

function status(response) {
    if (response.ok) {
        return response.json();
    } else if (response.status === 404) { // If the API *really* returns
        return [];                        // a 404 for "no breaches"
    } else {
        throw new Error(response.statusText);
    }
}

and then:

fetch('https://haveibeenpwned.com/api/v2/breachedaccount/' + account.value, {
        timeout: 1500,
        userAgent: 'test'
    })
    .then(status)
    .then(breaches => {
        // No `return` here, the chain isn't passed on and there
        // aren't any further resolution handlers
        breaches.forEach(check => {  // Or a for-of loop
            const span = document.createElement("span");
            span.innerHTML = `${check.Name}<br/>`;
            results.appendChild(span);
        });
    }).catch(error => {
        console.log('Request failed:', error);
    });

Separately: Your status function suggests that you don't realize that then (and catch) create new promises. There's no reason for your status function to create any promises if it's only going to be used as a then handler. It should just a return a value (the promise created by then will resolve with that value) or throw an error (the promise created by then will reject with that error):

// This is equivalent to your current `status` function (assuming it's always
// used as a `then` callback)
function status(response) {
    if (response.ok) { // if (response.status >= 200 && response.status < 300) {
        // all okay
        return response;
    } else if (response.status == 404) {
        // no breaches
        return response.statusText;
    } else if (response.status == 400) {
        // bad request
        return response.statusText;
    } else {
        throw new Error(response.statusText);
    }
}

(I removed the console.log lines that were after the return in each branch, as they were unreachable.)

How to handle this fetch() TypeError?

1 Answers