current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 2 down vote favorite
1

I've written an ES6 function using the new fetch() API and returning a new resolved promise with a data object or a rejected promise with an error subclass.

I'm pretty new to both ES6 and Promises, and it looks a little unwieldy, so I'm wondering if there's a better way. I'm also wondering about how this relates to the "explicit promise construction antipattern".

Any suggestions for how to improve or simplify?

function get(url) {
  console.log('Making fetch() request to: ' + url);

  let promise = new Promise((resolve, reject) => {
    fetch(url).then(response => {
      if (response.ok) {
        const contentType = response.headers.get('Content-Type') || '';

        if (contentType.includes('application/json')) {
          response.json().then(obj => {
            resolve(obj);
          }, error => {
            reject(new ResponseError('Invalid JSON: ' + error.message));
          });
        } else if (contentType.includes('text/html')) {
          response.text().then(html => {
            resolve({
              page_type: 'generic',
              html: html
            });
          }, error => {
            reject(new ResponseError('HTML error: ' + error.message));
          });
        } else {
          reject(new ResponseError('Invalid content type: ' + contentType));
        }
      } else {
        if (response.status == 404) {
          reject(new NotFoundError('Page not found: ' + url));
        } else {
          reject(new HttpError('HTTP error: ' + response.status));
        }
      }
    }, error => {
      reject(new NetworkError(error.message));
    });
  });

  return promise;
}

share|improve this question
up vote 2 down vote

A few comments:

1) fetch already returns a promise, which means this:

new Promise((resolve, reject) => {
  return fetch(url).then(response => {
    if (response.ok) {
      resolve(response)
    } else {
      reject(new Error('error'))
    }
  }, error => {
    reject(new Error(error.message))
  })
})

Is pretty much the same as:

fetch(url).then(response => {
  if (response.ok) {
    return response
  }
  return Promise.reject(Error('error'))
}).catch(error => {
  return Promise.reject(Error(error.message))
})

2) Keeping this in mind you can simplify your code with early returns and fewer branches:

function get(url) {
  return fetch(url).then(response => {
    if (response.ok) {
      const contentType = response.headers.get('Content-Type') || '';

      if (contentType.includes('application/json')) {
        return response.json().catch(error => {
          return Promise.reject(new ResponseError('Invalid JSON: ' + error.message));
        });
      }

      if (contentType.includes('text/html')) {
        return response.text().then(html => {
          return {
            page_type: 'generic',
            html: html
          };
        }).catch(error => {
          return Promise.reject(new ResponseError('HTML error: ' + error.message));
        })
      }

      return Promise.reject(new ResponseError('Invalid content type: ' + contentType));
    }

    if (response.status == 404) {
      return Promise.reject(new NotFoundError('Page not found: ' + url));
    }

    return Promise.reject(new HttpError('HTTP error: ' + response.status));
  }).catch(error => {
    return Promise.reject(new NetworkError(error.message));
  });
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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