susanBuck / e28-spring20

0 stars 0 forks source link

Computed property didn't finish API call during mounting #84

Closed tmussa1 closed 4 years ago

tmussa1 commented 4 years ago

I was able to clean up my code with local storage and avoid the child to parent props/events. Now I have some issue with a computed property not finishing the API call during mounting and always returning a default value. Please help me out https://github.com/tmussa1/p2/blob/master/src/components/Stats.vue

susanBuck commented 4 years ago

FYI - Looking at your code now; will follow-up shortly with more details.

susanBuck commented 4 years ago

To understand what's going wrong, let's look at your winCount computed property - here you try to return the results of getWinCount:

winCount: {
      get() {
        return getWinCount(this.playerName); // <---
      }, 
      set(newCount) {
        this.winCount = newCount;
      }
    },

Ad here's what that getWinCount function does:

export function getWinCount(playerName) {
  if (!firebase.apps.length) {
    firebase.initializeApp(config);
  }

  let api = firebase.firestore();
  let winCount = 0; // <-- You initialize winCount as 0

  filter('users', 'name', playerName).then(function(result) {
    let docRef = result.shift();
    api
      .collection('users')
      .doc(docRef.id)
      .get()
      .then(function(doc) {
        winCount = doc.data().wins; // <-- You update winCount
      })
      .catch(function(error) {
        console.log('Error getting documents: ' + error);
      });
  });
  return winCount; // <-- You return winCount, expecting it to be the updated value, but it will always be 0
}

In this function you define winCount initially as 0, then you invoke filter with the intention of updating winCount, and finally you return winCount.

The problem is, at the point of that return, winCount will always still be 0. The reason is because filter is an asynchronous method. This mean it does not get an instant response. Instead there's a delay - even if it's a fraction of a section - it's still a delay because it's calling the API to get the data.

So while the JS code continues on to that return winCount statement, the filter method is still doing its work, and by the time it actually updates winCount, the function has already returned its value, which was 0.

To address this, you want to not return winCount/the resulting value. Instead you want to return filter itself, which is actually a Promise.

Then, when you invoke getWinCount you need to remember you're getting a Promise from this function and treat it as such.

So instead of something like:

// Here we expect an instant value to be returned from getWinCount, which won't happen because getWinCount returns a Promise
this.winCount = getWinCount(this.playerName);

You'd need to say:

// Here we understand getWinCount returns a Promise and so we chain on a `then` method to it to handle the response of that promise
getWinCount(this.playerName).then(response => {
    this.winCount = response;
});

For more on async methods/Promises/etc, I suggest checking out this resource: https://javascript.info/async

Specifically, https://javascript.info/promise-basics

All of that being said, in debugging your code to understand what is going on and get it working, I did some refactoring and suggest the following:

  1. Simplify values in Stats.vue...instead of having a winCount/lossCount and a updatedWinCount and updatedLossCount, just have a winCount/lossCount
  2. Instead of updating winCount/lossCount via computed properties, do it in the mounted method.
  3. In getWinCount/getLossCount you're using filter to get the collection, then you're doing another query to get the collection data; this isn't necessary as the filter makes that data accessible to you.
  4. Instead of having separate getWinCount and getLossCount function, combine them to one function that returns both the winCount and lossCount. They both have to ping the API for the same doc, so there's no sense making that ping twice.

Factoring in the above suggestions, here's the resulting code I came up with:

In callFirebase.js:


export async function getCounts(playerName) {

    // Note that this function is returning a Promise, not an actual value
    // This is important to understand in the code from which we're invoking this function
    return filter('users', 'name', playerName).then(function (result) {

        let docRef = result.shift();
        let wins = docRef.data().wins;
        let losses = docRef.data().losses;

        return {
            wins: wins,
            losses: losses
        }
    });
}

In Stats.vue:

import NavBar from './NavBar.vue';
import Chart from 'chart.js';
import winningStatistics from './../../public/winningStatistics';
import { getCounts } from '../../public/callFirebase';

/* eslint-disable no-unused-vars */
export default {
    data: function() {
        return {
            winningStatistics: winningStatistics,
            winCount: null,
            lossCount: null
        };
    },
    methods: {
        formChart: function(statistics) {
            let chartCanvas = document.getElementById('player-statistics');
            let playerChart = new Chart(chartCanvas, {
                type: statistics.type,
                data: statistics.data,
                options: statistics.options
            });
        },
        playGame: function() {
            this.$router.push({
                path: '/categories'
            });
        }
    },
    mounted() {

        // getCounts is an async function - it returns a promise; in order to get it's results, 
        // we have to capture it via a `then` callback (ref: https://javascript.info/promise-basics#then)
        getCounts(this.playerName).then(response => {

            // response is an object with values for wins and losses
            console.log('response from getCounts:', response);

            this.winCount = response.wins;
            this.lossCount = response.losses;

            this.winningStatistics.data.datasets[0].data[0] = this.winCount;
            this.winningStatistics.data.datasets[0].data[1] = this.lossCount;

            this.formChart(this.winningStatistics);

            console.log(
                'winCount: ',
                this.winCount,
                'lossCount: ',
                this.lossCount,
                ' playerName: ',
                this.playerName
            );
        });
    },
    components: {
        NavBar
    },
    computed: {
        playerName: function() {
            return localStorage.getItem('player');
        }
    }
};

Hope the above info helps; I know it's a lot so don't hesitate to let me know if you have any follow-up questions!

tmussa1 commented 4 years ago

@susanBuck this is extremely helpful. I refactored my code and working as expected