Exercise to fetch data from JSON files using ES6 promises

Question

I did an exercise (self-imposed) promise fetching some data from placeholder JSON files on https://jsonplaceholder.typicode.com/. The exercise in my opinion is finished, but I can't shake the feeling that the code is as dirty as code can be. Please check it out and point me into the right direction and tell me what I did wrong.

The exercise can be found here: http://codepen.io/kresimircoko/pen/vggPmJ

'use strict';
const root = 'https://jsonplaceholder.typicode.com/';
const postList = document.querySelector('#post-list');
console.clear();
const commentsPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'comments');
 xhr.send();
});
const usersPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'users');
 xhr.send();
});
const postsPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'posts');
 xhr.send();
});
// Posts
postsPromise
 .then(data => {
 const posts = JSON.parse(data); // Contains our posts
 usersPromise
 .then(data => {
 const users = JSON.parse(data); // Contains our users
 commentsPromise
 .then(data => {
 const comments = JSON.parse(data);
 displayPosts(posts, users, comments);
 });
 });
 }).catch(error => { console.log(error); });
// Users
function displayPosts(posts ,users, comments) {
 let output = '';
 posts.map(post => {
 output += `
 <li class="post"> 
 <h4> ${displayUserName(users, post)} </h4>
 <h3> ${post.title} </h3>
 <p> ${post.body} </p>
 <div class="comment-count"><span> ${displayCommentAmount(comments, post)} comments</span></div>
 </li>
 `;
 });
 postList.innerHTML = output;
}
function displayUserName(users, post) {
 return users.map(user => {
 if (user.id === post.userId) {
 return user.name;
 }
 }).join('');
};
function displayCommentAmount(comments, post) {
 const commentArray = comments.filter(comment => {
 return comment.postId === post.id;
 });
 return commentArray.length;
}

Answer 1

There are lots of things you can do:

1. Use a parameterized function to create the XHR promises, since your three functions are identical other than what they're requesting
2. Use Promise.all, since none of the requests relies on information from previous requests and they can run in parallel
3. Move the JSON parsing into the functions retrieving the data, rather than making the consumer of them do it

And a bugfix, I think: The response text is this.responseText, not this.response, in the XHR load callback.

That would let us replace all of this:

const commentsPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'comments');
 xhr.send();
});
const usersPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'users');
 xhr.send();
});
const postsPromise = new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 resolve(this.response);
 };
 xhr.onerror = reject;
 xhr.open('GET', root + 'posts');
 xhr.send();
});
// Posts
postsPromise
 .then(data => {
 const posts = JSON.parse(data); // Contains our posts
 usersPromise
 .then(data => {
 const users = JSON.parse(data); // Contains our users
 commentsPromise
 .then(data => {
 const comments = JSON.parse(data);
 displayPosts(posts, users, comments);
 });
 });
 }).catch(error => { console.log(error); });

with this:

const requestJSON = objType => {
 return new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 try {
 resolve(JSON.parse(this.responseText));
 }
 catch (e) {
 reject(e);
 }
 };
 xhr.onerror = reject;
 xhr.open('GET', root + objType);
 xhr.send();
 });
};
Promise.all([requestJSON('posts'), requestJSON('users'), requestJSON('comments')])
 .then(results => {
 displayPosts(...results);
 })
 .catch(error => {
 console.log(error);
 });

So shorter, simpler, and more centralized for debugging, not repeating yourself, etc.

If you want the requests done in series instead of in parallel even though they don't have to be, just change the Promise.all part to:

requestJSON('posts')
 .then(posts => requestJSON('users')
 .then(users => requestJSON('comments')
 .then(comments => {
 displayPosts(posts, users, comments);
 })
 )
 )
 .catch(error => {
 console.log(error);
 });

(I tried to keep that in line with your line break/indentation style...)

Final result (with parallel requests):

'use strict';
const root = 'https://jsonplaceholder.typicode.com/';
const postList = document.querySelector('#post-list');
console.clear();
const requestJSON = objType => {
 return new Promise((resolve, reject) => {
 const xhr = new XMLHttpRequest();
 xhr.onload = function() {
 try {
 resolve(JSON.parse(this.responseText));
 }
 catch (e) {
 reject(e);
 }
 };
 xhr.onerror = reject;
 xhr.open('GET', root + objType);
 xhr.send();
 });
};
Promise.all([requestJSON('posts'), requestJSON('users'), requestJSON('comments')])
 .then(results => {
 displayPosts(...results);
 })
 .catch(error => {
 console.log(error);
 });
// No changes from here on out...
function displayPosts(posts ,users, comments) {
 let output = '';
 posts.map(post => {
 output += `
 <li class="post"> 
 <h4> ${displayUserName(users, post)} </h4>
 <h3> ${post.title} </h3>
 <p> ${post.body} </p>
 <div class="comment-count"><span> ${displayCommentAmount(comments, post)} comments</span></div>
 </li>
 `;
 });
 postList.innerHTML = output;
}
function displayUserName(users, post) {
 return users.map(user => {
 if (user.id === post.userId) {
 return user.name;
 }
 }).join('');
};
function displayCommentAmount(comments, post) {
 const commentArray = comments.filter(comment => {
 return comment.postId === post.id;
 });
 return commentArray.length;
}

• woxxom: += makes the js engine intern a lot of progressively increasing intermediate strings, which is terribly inefficient, so it's better to use .map to generate an array then .join('') it.
• T.J. Crowder: @wOxxOm: That's more of a comment on the question (or even part of an answer) than a comment on this answer. I limited myself to addressing the promises part of things (based on the title of the question). (It's also not always better to build an array and join it, though often it is.)
• Krešimir Čoko: @wOxxOm thank you for that. That part of the code also feels really brittle so I was looking at feedback for that part too :)
• T.J. Crowder: @KrešimirČoko: What I missed in there is that you were already using map. At the very least you'd want to either use map and join as w0xx0m suggested, or use forEach rather than map (since you're not using the array it creates). But you might consider any of several templating libraries (or even going further into MVC land with React, Angular, Vue, or ...). :-)
• Krešimir Čoko: @T.J.Crowder I will try it with forEach and see where I end up. The current version with the map isn't flexible at all. Time to work more on it :) Thank you for the elaborate answer