TypeScript fetch wrapper class with localStorage cache

Question

I'm learning TypeScript while doing my portfolio, and I wrote a class to make a fetch request to the API that holds my data.

It is working fine. I use localStorage to set a cache to avoid calling the API often as the data won't change.

I know I could improve this a lot, and haven't used many TypeScript features, so it would be great to have some pointers on how to improve this class generally, and also how to improve it by adding some more TypeScript features.

This is the class:

export interface PortfolioData {
  basics: Basics;
  projects?: Project[];
}

interface Cache {
  portfolio: PortfolioData;
  expiration: number;
}

class Portfolio {
  private url = 'https://gitconnected.com/v1/portfolio/mauriciorobayo';
  private cacheDurationInMilliseconds: number;

  constructor(cacheDurationInMinutes: number) {
    this.cacheDurationInMilliseconds = cacheDurationInMinutes * 60 * 1000;
  }

  private getCache(): Cache | undefined {
    const data = localStorage.getItem('portfolio');

    if (!data) {
      return;
    }

    const cache = JSON.parse(data);

    if (Date.now() > cache.expiration) {
      return;
    }

    return cache;
  }

  private setCache(data: Cache) {
    localStorage.setItem('portfolio', JSON.stringify(data));
  }

  async getPortfolio(): Promise<PortfolioData> {
    const cache = this.getCache();

    if (cache) {
      return cache.portfolio;
    }

    const response = await fetch(this.url);
    if (response.ok) {
      const portfolio = await response.json();

      this.setCache({
        portfolio,
        expiration: Date.now() + this.cacheDurationInMilliseconds,
      });

      return portfolio;
    }
    throw new Error(response.statusText);
  }
}

export default Portfolio;

Answer 1

The review will be focused on readability and testability.

Naming

I read (I think it was in Clean Code) that variables should not have a postfix like Data and Information and instead we should give things a name that they represent.

PortfolioData is actually a Portfolio while your current Portfolio is more a PortfolioRepository or PortfolioCollection.

Additionally I couldn't figure out, what Basic could mean and I looked up the json-representation of the gitconnected api:

"basics": {
   "name": "...",
   "picture": "https://avatars2.githubusercontent.com/u/2121481?v=4",
   "label": "...",
   "headline": "...",
   "summary": "...",
   "website": "...",
   "blog": "...",
   "yearsOfExperience": 2,
   "id": "...",
   "username": "...",
   "karma": 22,
   "email": "...",
   "region": "...",
   "location": {},
   "phone": "...",
   "followers": 63,
   "following": 94,
   "profiles": []
}

For me a more descriptive name would be User or Owner. A Portfolio has an Owner and a PortfolioRepository stores Portfolio-Objects:

export interface Portfolio {
  owner: Owner;
  projects?: Project[];
}

interface Cache {
  portfolio: Portfolio;
  expiration: number;
}

class PortfolioRepository { /* ... */ }

Concerns

The class Portfolio handles multiple concerns. There are two principles which can be named to emphasize the importance for separation:

• Separation of Concerns (SoC)
• Single Responsibility Principle (SRP)

The concerns are

• fetching (await fetch(this.url))
• caching (this.setCache(...))
• querying (async getPortfolio(): Promise<PortfolioData>)

When we separate the concerns we could accomplish something like

interface PortfolioRepository {
    get(): Promise<Portfolio>;
}

class ApiPortfolioRepository implements PortfolioRepository {
    constructor(private cache: Cache, 
                private api: API) {}

    async get(): Promise<Portfolio> {
        if (this.cache.containsPortfolio()) {
            return this.cache.get();
        }

        const portfolio = await this.api.fetch();
        this.cache.update(portfolio);
  
        return portfolio;
    }
}

This has multiple advantages. Beside the descriptive and shorter method body we can accomplish through the dependency injection via the constructor a better way to test the class.

Now the PortfolioRepository does not need to know about how caching works like the cacheDurationInMilliseconds and it does not need to know the http endpoint any more.

If we want to test the class we could simply mock the api (for exampe with mocha and chai):

import { assert } from "chai";

suite('API Portfolio Repository', () => {

    const cache = ...;
    const fakeAPI = new FakeAPI();

    test('when API returns no portfolio -> return undefined', () => {
        const repository = new ApiPortfolioRepository(cache, fakeAPI.withoutResponse);
        
        const portfolio = repository.get();

        assert.isUndefined(portfolio);
    });

    test('when API returns a portfolio -> return portfolio', () => {
        const repository = new ApiPortfolioRepository(cache, fakeAPI.withResponse);
        
        const portfolio = repository.get();

        assert.deepEqual(portfolio, fakeAPI.portfolio);
    });

})