Refactor Ability Repository for proper expiry

[gdirk07]

When we set a local storage for expiry, it creates a new record, while this is nice for big blobs (all pokemon), we'd need to create one for each individual object (specific pokemon, ability, move etc). I feel its better to store this in the record itself rather than create a new UNIQUE entry because otherwise we'd need an expiry record in the table for EACH object. While this is an approach, I think storing it in the object makes more sense when checking since we can just look at that table record's expiry, and not look at a second record that loosely points to it.

What I did was rewrote the repository to STORE the expiry with the object, and will update whenever an update is made to it (use case? not sure yet, maybe if the app/api makes an update that we need to run in the background we can update the repository). A further update to PokemonRepository would need to be made because individual pokemon lookup is not using it right now, only the blob at the start of the application run is.

What I need to verify

1. When it expires, would refetching/setting in the repository overwrite the existing value? Do we need to clear it before hand?
2. What are the proper use cases of resetting the expiry (if a user looks at a pokemon with an ability that exists in the repo, do we want to update that expiry? leave it alone?

[jeremy-jtlo]

Going to give thoughts to the expiry stuff below:

• When it expires, would refetching/setting in the repository overwrite the existing value? Do we need to clear it before hand?

My initial thought is yes, we do want to overwrite the existing value if the expiry timestamp runs out. That's the general law behind expiring data, that the app acknowledges that data can go stale and may need a re-fetch. Abilities don't change very often for Pokemon though, so I could see having a long timeout on our abilities.

Hesitant to say "no timeout" because the idea of never clearing a cache is unsettling to me.

• What are the proper use cases of resetting the expiry (if a user looks at a pokemon with an ability that exists in the repo, do we want to update that expiry? leave it alone?

If we're looking at 2 that share an ability (something common like "intimidate"), then I'd say just get it from the repo if it's still a valid entry.

[jeremy-jtlo]

Another thing I forgot to mention: when a PokemonDTO is created by the factory, the factory should be confident that the AbilityService is giving it good data. The service itself will do the work of checking the timestamps and re-fetching when necessary, because that's its job.

All the factory cares about is getting an AbilityDTO to feed to the Pokemon it's creating. Neither the factory or the resulting Pokemon should know if an ability came from the cache or not.

[gdirk07]

Hesitant to say "no timeout" because the idea of never clearing a cache is unsettling to me.

Agreed

If we're looking at 2 that share an ability (something common like "intimidate"), then I'd say just get it from the repo if it's still a valid entry.

Sorry, I mean do we "reset the expiry?" Right now it works as you stated, however should we update the expiry? My thoughts are that if a user is looking through pokemon with a certain ability in common "intimidate" then we wouldn't want it to expire, i.e. keep it cached. I can see the argument against this in that potentially never letting data go stale could be an issue if it were to somehow update.

[gdirk07]

Another thing I forgot to mention: when a PokemonDTO is created by the factory, the factory should be confident that the AbilityService is giving it good data. The service itself will do the work of checking the timestamps and re-fetching when necessary, because that's its job.

All the factory cares about is getting an AbilityDTO to feed to the Pokemon it's creating. Neither the factory or the resulting Pokemon should know if an ability came from the cache or not.

If this is the case, then should we not merge it until its resolved? Or are you planning on moving the task to another issue to resolve.

[jeremy-jtlo]

I just looked at the factory code, it works but if we want to be purists it'd be better if AbilityService could just provide the necessary abilities via one method.

So instead of:

abilities: data.abilities.map((abilityData) => [
  this.abilityService.createAbilityFromStub(abilityData),
  abilityData.is_hidden,
]),

It would look something like:

abilities: this.abilityService.getAbilities(data.abilities) // all the magic happens in the service method

Could flag it for improvement with an issue but for now I don't think it's a huge problem.