3
\$\begingroup\$

I am currently working on a simple app to check the stats of popular javascript frameworks. For this I am using the Github API to get certain data sets and have come close to finishing my project.

const frameworks = [
  {
    name: "angular",
    logo: "img/angular.png",
    url: "https://api.github.com/repos/angular/angular.js"
  }
  /* 
  {
    name: "ember",
    logo: "img/ember.png",
    url: "https://api.github.com/repos/emberjs/ember.js"
  },
  {
    name: "react",
    logo: "img/react.png",
    url: "https://api.github.com/repos/facebook/react"
  },
  {
    name: "vue",
    logo: "img/vue.png",
    url: "https://api.github.com/repos/vuejs/vue"
  }
  */
];

const frameworkData = url => {
  return fetch(url)
    .then(resp => {
      return resp.json();
    })
    .then(data => {
      return {
        data: {
          name: data.name,
          forks: data.forks_count,
          stars: data.stargazers_count,
          issues: data.open_issues_count
        }
      };
    });
};

const setup = () => {
  let now = new Date();
  const row = document.getElementById("row");

  frameworks.forEach(framework => {
    const promises = [];
    promises.push(frameworkData(framework.url));

    Promise.all(promises).then(data => {
      const obj = data[0].data;
      console.log(obj);
      const card = `
      <div class="col-lg-3">
        <div class="card" style="width: 18rem;">
          <img src="${
            framework.logo
          }" alt="Card image cap" height="180" width="200">
          <div class="card-body">
            <h5 class="card-title">${framework.name}</h5>
          </div>
          <ul class="list-group list-group-flush">
            <li class="list-group-item">Watchers: ${obj.forks}</li>
            <li class="list-group-item">Stars: ${obj.stars}</li>
            <li class="list-group-item">Commits: ${obj.issues}</li>
          </ul>
        </div>
      </div>`;
      row.innerHTML += card;
    });
  });
};

setup();

Being this is one of my first javascript 'apps' I feel it was done to the best of my ability. However, I feel there is a code smell in here that I have been trying to figure out how to fix but have not.

As you may be able to see in the frameworks for each loop, I am settings this variable const obj = data[0].data;. To me this is strange and I could not seem to find away around it. This is done because the single object for that API call is being returned in an array and I was not sure how to get that data other than calling the first index on the array since it will always be 0. Is there a better way to do this? How was my first javascript app?

\$\endgroup\$

3 Answers 3

Reset to default
1
\$\begingroup\$

The "data" property of the object returned from .then() chained to fetch() is not necessary as it is not used. Simply returning the object will suffice.

Use .map() instead of .forEach() to process array of fetch() calls made when frameworks is iterated.

Promise.all() returns an array of resolved Promise objects in the same order as the elements within the iterable passed to the function, whether the element is a Promise or not. Note also, that a single error or rejected Promise stops Promise.all() from further iteration.

Chain .catch() to both Promise.all() and setup() to include error handling for the asynchronous function calls, and return a value from .then() chained to Promise.all().

<!DOCTYPE html>
<html>

<head>
</head>

<body>
  <div id="row"></div>
  <script>
    const frameworks = [{
      name: "angular",
      logo: "img/angular.png",
      url: "https://api.github.com/repos/angular/angular.js"
    }, {
      name: "ember",
      logo: "img/ember.png",
      url: "https://api.github.com/repos/emberjs/ember.js"
    }, {
      name: "react",
      logo: "img/react.png",
      url: "https://api.github.com/repos/facebook/react"
    }, {
      name: "vue",
      logo: "img/vue.png",
      url: "https://api.github.com/repos/vuejs/vue"
    }];

    const handleError = err => {throw err};

    const frameworkData = url =>
      fetch(url)
      .then(resp => resp.json())
      .then(data => 
        ({
          name: data.name,
          forks: data.forks_count,
          stars: data.stargazers_count,
          issues: data.open_issues_count
        })
      ).catch(handleError);

    const setup = (
      row = document.getElementById("row")
    , now = new Date()
    ) => 
      Promise.all(
        frameworks.map(({url,logo,name:n}) => 
          frameworkData(url)
          .then(({forks,stars,issues}) => 
            row.innerHTML += `
              <div class="col-lg-3">
                <div class="card" style="width: 18rem;">
                  <img src="${logo}" alt="Card image cap" height="180" width="200">
                  <div class="card-body">
                    <h5 class="card-title">${n}</h5>
                  </div>
                  <ul class="list-group list-group-flush">
                    <li class="list-group-item">Watchers: ${forks}</li>
                    <li class="list-group-item">Stars: ${stars}</li>
                    <li class="list-group-item">Commits: ${issues}</li>
                  </ul>
                </div>
              </div>`))
            ).catch(handleError);

    setup()
    .then(() => console.log("done fetching frameworks data"))
    .catch(err => console.error(err));
  </script>
</body>

</html>

\$\endgroup\$
1
\$\begingroup\$

Async would be better.

How time flies, the promise API was the bees knees, but now it looks old and messy. Async functions are the way to go.

This review is just a quicky as I found points and there is a very basic example that address the following points.

  • Transformer?

I don't understand why you transform the data after you get it (example you change forks_count to forks, and so on). This looks like a unnecessary step as you only use the data to fill a card and then you dump it.

  • Promise.all for related data.

Only use Promise.all when the various data items you are waiting on are dependent on each other.

  • Promise is dead, Long live Async.

async functions are much better than Promise. Async functions let you use promises without the need to create promises, define the callback functions that service the them, and write then over and over again. async functions are a promise without all the pleading...

  • Markup where it belongs.

Use a HTML <template> element to define the card. Javascript strings are not the place for HTML, and adding content to the page via markup and element.innerHTML is about the slowest way to add content to a page.

HTML is designed to hold data. Use an element's data property to associate data to content. Eg the <li> elements have the property data-field which holds the name of data field it will display <li data-field="forks_count">Watchers:</li>

  • Construct the page, then fill it.

Laying out the page as you get data makes the page feel sluggish, prevents the client navigating any existing content as reflows and new content move everything about.

Use placeholders for the cards and always give feedback (spinners show errors).

  • Don't repeat yourself (DRY)

The array frameworks repeats the same API url "https://api.github.com/repos/" for each item (very easy to miss a typo in repeated code). Each item also repeats the framework name 3 times.

Example

This is a very basic example using async. The HTML and the array frameworks is now stored as markup.

The function createCard handles the request and parses the JSON. When the data is ready, using a template it fills relevant fields and returns the new card.

The card is added to the DOM when the async function resolves, or there is an error.

The entry point is the outer function addCards that will add cards to the array of elements. (there is no vetting)

function addCards(elements) {
  async function createCard({name, ext = ""}) {
    const card = document.importNode(frameworkCard.content, true);        
    const fields = card.querySelectorAll("li");
    const fillEl = (qry, prop, el=card.querySelector(qry))=>el && Object.assign(el, prop);

    const data = await (await fetch("https://api.github.com/repos/" + name + ext)).json();
    fillEl("img", {src : `img/${name}.png`});
    fillEl(".card-title", {textContent : name});
    for (const el of fields) { el.textContent += data[el.dataset.field] }
    return card;
  }        
  for (const el of elements) {
    createCard(el.dataset)
      .then(card => (el.textContent = "", el.appendChild(card)))
      .catch(() => el.textContent = "Error");
  }
}
addCards(document.querySelectorAll(".card-container"));

Where the cards will go.

The array of elements that are the placeholders for the cards. They also contain the data that is required to make the API calls for that card.

<div class="card-container" data-name="angular" data-ext=".js">Loading</div>    
<div class="card-container" data-name="ember" data-ext=".js">Loading</div>
<div class="card-container" data-name="react">Loading</div>
<div class="card-container" data-name="vue">Loading</div>

The template

that is used to create a card. It is automatically not part of the page layout thus does not incur overhead until you import it and add it to the DOM.

<template id="frameworkCard">
    <img height="180" width="200">
    <h5 class="card-title"></h5>
    <ul class="list-group list-group-flush">
        <li data-field="forks_count">Watchers:</li>
        <li data-field="stargazers_count">Stars:</li>
        <li data-field="open_issues_count">Commits:</li>
    </ul>
</template>
\$\endgroup\$
10
  • \$\begingroup\$ "Promise is dead"? When did Promise become "dead"? "async functions are much better than Promise" An async function still returns a Promise. How are you defining "better"? \$\endgroup\$ Commented Dec 6, 2018 at 2:11
  • \$\begingroup\$ @guest271314 Stillborn (well on life support) due to the need to use a callback to get data or pass data from one promise to the next. Life support turned off when Async was born "An async function still returns a Promise" I did point that out. \$\endgroup\$ Commented Dec 6, 2018 at 9:47
  • \$\begingroup\$ Since you insist on stating that Promise is "dead", how would you replace new Promise(resolve=>setTimeout(resolve, 1000, 1)) with an async function without using Promise constructor? \$\endgroup\$ Commented Dec 6, 2018 at 16:19
  • 1
    \$\begingroup\$ @guest271314 OK without using Promise constructor. How about (async () => Promise.resolve({then(t) { setTimeout(t, 1000) }}))().then(()=>log("One second")) \$\endgroup\$ Commented Dec 6, 2018 at 17:31
  • \$\begingroup\$ While the code does not use Promise constructor, the code still uses Promise.resolve(). The async function used in the code is not necessary to achieve the same result using only Promise.resolve() as you demonstrate or Promise constructor and .then(). Does that not prove that Promise is not "dead"? \$\endgroup\$ Commented Dec 7, 2018 at 2:42
1
\$\begingroup\$

Your questions

How was my first javascript app?

Overall the code seems somewhat fine for a first attempt (barring the flaw mentioned in the first section below). It isn't difficult to read though it does have a few extra levels of nesting which could be cleaned up with abstraction code functions out to named functions.

Is there a better way to do this?

As was suggested by others, there isn't really a point to returning the data nested under a key "data". Refer also to the other suggestions below for simplifying the code.

Other Feedback/Suggestions

Likely implementation flaw

frameworks.forEach(framework => {
  const promises = [];
  promises.push(frameworkData(framework.url));
  Promise.all(promises)

This code will call Promises.all() passing an array with a single promise, which seems pointless. While there is no rule that says there has to be two or more promises it basically defeats the purpose.

Presumably you meant to push the promises into an array and pass the array with all promises to the call to Promise.all().

Creating array of promises

One could use Array.prototype.map() to construct the array. 

const promises = frameworks.map(framework => frameworkData(framework.url));

utilizing data

If you take the advice above, the first argument in a callback to .then() chained to Promise.all() should be an array of the data objects returned by the last promise callback within each call to frameworkData(), and there won’t be any reference to ‘frameworkso you might either need to pass that object along through the call toframeworkData` or look it up by index.

Template inline

The template literal used to define card could be moved out to a template tag with slots or a <script type="text/template”> that can be referenced by id attribute in the code. For more of an explanation of this, refer to this post, as well as an example application of it in this post. The requirements for this may be a bit complex for a beginner but the motivation is to keep the markup (i.e. HTML) separate from the business logic (i.e. JavaScript).

A script tag like this could be added to the body within the HTML:

<script type=“text/template” id=“card-template”>
    <div class="col-lg-3">
        <div class="card" style="width: 18rem;">
            <img src="${logo}" alt="Card image cap" height="180" width="200">
            <div class="card-body">
                <h5 class="card-title">${name}</h5>
            </div>
            <ul class="list-group list-group-flush">
                <li class="list-group-item">Watchers: ${forks}</li>
                <li class="list-group-item">Stars: ${stars}</li>
                <li class="list-group-item">Commits: ${issues}</li>
            </ul>
        </div>
    </div>
</script>

Then that could be referenced via the JavaScript:

const cardTemplate = document.getElementById('card-template').innerHTML;

You would then need interpolate the string, and unfortunately that isn't in a template literal anymore. However, an approach like outlined in this post should suffice.

The following function added in the JavaScript like this:

String.prototype.interpolate = function(params) {
  const names = Object.keys(params);
  const vals = Object.values(params);
  return new Function(...names, `return \`${this}\`;`)(...vals);
}

Would allow for a line like this:

row.innerHTML += cardTemplate.interpolate(data);

Simplifying single line returns in arrow statements

Arrow functions that simply have one line in the block, like:

resp => {
  return resp.json();
}

Can be simplified to a single line without the block and return:

resp => resp.json()

Unused variable

The following variable doesn't appear to be utilized:

let now = new Date();

It can be removed unless you plan to utilize it in the future. If the value never changes, it would be wise to declare it with const.

Putting it all together

See simplified code below.

String.prototype.interpolate = function(params) {
  const names = Object.keys(params);
  const vals = Object.values(params);
  return new Function(...names, `return \`${this}\`;`)(...vals);
}
const frameworks = [{
    name: "angular",
    logo: "img/angular.png",
    url: "https://api.github.com/repos/angular/angular.js"
  },
  {
    name: "ember",
    logo: "img/ember.png",
    url: "https://api.github.com/repos/emberjs/ember.js"
  },
  {
    name: "react",
    logo: "img/react.png",
    url: "https://api.github.com/repos/facebook/react"
  },
  {
    name: "vue",
    logo: "img/vue.png",
    url: "https://api.github.com/repos/vuejs/vue"
  }
];

const frameworkData = framework => {
  return fetch(framework.url)
    .then(resp => resp.json())
    .then(data => {
      return {
          logo: framework.logo,
          name: data.name,
          forks: data.forks_count,
          stars: data.stargazers_count,
          issues: data.open_issues_count
      };
    });
};

const setup = () => {
  const row = document.getElementById("row");
  const cardTemplate = document.getElementById('card-template').innerHTML;
  const promises = frameworks.map(framework => frameworkData(framework));
  Promise.all(promises).then(data => {
    row.innerHTML = data.map(obj => cardTemplate.interpolate(obj));
  });
};

document.addEventListener('DOMContentLoaded', setup);
<div id="row"></div>
<script type="text/template" id="card-template">
  <div class="col-lg-3">
    <div class="card" style="width: 18rem;">
      <img src="${logo}" alt="Card image cap" height="180" width="200">
      <div class="card-body">
        <h5 class="card-title">${name}</h5>
      </div>
      <ul class="list-group list-group-flush">
        <li class="list-group-item">Watchers: ${forks}</li>
        <li class="list-group-item">Stars: ${stars}</li>
        <li class="list-group-item">Commits: ${issues}</li>
      </ul>
    </div>
  </div>
</script>

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.