Move Module Cache outside of resource

[tintinthong]

This PR moves cache into service rather than residing in the resource

The original motive of this is so that other resources can consume this if needed. Here is the comment #656 (comment)

[github-actions]

Test Results

394 tests  ±0   387 ✔️ ±0   6m 1s ⏱️ +18s
    1 suites ±0       7 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit c9c9252. ± Comparison against base commit 6108d83.

♻️ This comment has been updated with latest results.

[tintinthong]

It seems that this was not the issue breaking test altho I thought it was at one point so I just reverted here

[habdelra]

this seems like a bad assumption. how do you know that URL that is being requested is for card-source? This service is called "realm-info-service" it doesn't seem to indicate that this is only for card sources. Perhaps the accept header should be part of the parameters if this is necessary

[tintinthong]

if I were to use the realmInfo provider, I'd need to do a fetch for card source. I can use a different method and a different cache so we will not make this bad assumption. But the method that is called fetchExtension can be used for CardSource

[habdelra]

having these implicit requirements on the callers feels like it could be the source of bugs if people don't clearly understand how to use this API. I would suggest that just make the accept header explicit by requiring callers to specify it when they provide the URL to get the realm info. the accept header is part of how you ask for resources from the realm server anyways--let's just make that clear in our API for this service

[habdelra]

I don't understand why you need this service--isn't this what realm info service is doing? why can't you just move the cache into that service?

[tintinthong]

I am fine with that. I considered that initially because the things it does is so similar to the realmInfoProvider but just felt that the naming purpose wasnt quite right. I can go ahead with that -- it does reduce code and caches

[habdelra]

is this to cache 302 redirects? seems like a premature optimization to me. Until we know that is a real issue i would suggest that we should eliminate these lines of code.

[habdelra]

let's not reinvent the wheel and use the realm-info service here

[habdelra]

just to be clear, my comment around module scope did not mean move this into a service. It simply meant move the cache one block higher in the scope of the resource you already had (supeer simple change), so that instead of having:

export class CardType extends Resource<Args> {
  moduleMetaCache: Map<string, { extension: string; realmInfo: RealmInfo }> = new Map();
}

you would have:

const moduleMetaCache: Map<string, { extension: string; realmInfo: RealmInfo }> = new Map();

export class CardType extends Resource<Args> {
  ...
}

that way the cache is shared by all instances of the card type resource instead of only within a single instance. but i can see that if you want the cache to be shared by things that are not CardType resources, then caching in a service makes sense. but given what actually consumes this, i think making this a service is premature at this point

[tintinthong]

just to be clear, my comment around module scope did not mean move this into a service. It simply meant move the cache one block higher in the scope of the resource you already had (supeer simple change), so that instead of having:

export class CardType extends Resource<Args> {
  moduleMetaCache: Map<string, { extension: string; realmInfo: RealmInfo }> = new Map();
}

you would have:

const moduleMetaCache: Map<string, { extension: string; realmInfo: RealmInfo }> = new Map();

export class CardType extends Resource<Args> {
  ...
}

that way the cache is shared by all instances of the card type resource instead of only within a single instance. but i can see that if you want the cache to be shared by things that are not CardType resources, then caching in a service makes sense. but given what actually consumes this, i think making this a service is premature at this point

Ok I have made a change based upon this ultimately just moving the cache into module scope. I didn't use the realm info service and didn't create another service

[habdelra]

nice. very tight (low LOC) 👍