Decorator seems to cache resolved service for all future instances, breaking use in tests

[bwbuchanan]

ember-polaris-service/ember-polaris-service/src/decorator/stage-three.ts

Lines 280 to 284 in b5d0f30

	if (value === undefined) {
	value = service(this, name);
	}
	return value;

I ran into an issue writing acceptance tests, where I was providing a different mock implementation of a service in each test. The service was injected (using override) into a route under test.

What I found was that the mock service was successfully injected into the route for the first test, but when the route instance was reconstructed for the second test, the service was still resolving to the one injected by the first test.

Changing the code quoted above to simply return service(this, name); resolved the problem.

For reference, my route is doing something like this:

import { service } from 'ember-polaris-service/compat';
import Supabase from '#app/services/supabase';

export default class Login extends Route<LoginModel> {
  @service(Supabase) declare supabase: Supabase;
  @service(RouterService) declare router: RouterService;

  ...
}

And my tests are doing something like this:

import { override, singleton } from 'ember-polaris-service';
import Supabase from '#app/services/supabase';

...

const supabase = new SupabaseTest();
supabase.getSession = ....
override(this.owner, Supabase, singleton(supabase));

await visit('/login');

[bwbuchanan]

@chancancode What do you think about this?

I forgot about this and then ran into this same issue again and spent half the day debugging it. 🤦

[bwbuchanan]

I think if we're going to cache the return value of the getter that it needs to be in a WeakMap keyed by this. Otherwise the return value gets cached the first time the getter is used by any instance of the class, when the intent is that it get cached for that specific instance of the class.

[chancancode]

Oh hey, sorry I missed this. I would think for now, we can probably get rid of the caching at the decorator level, unless it's proven to be an important optimization, what do you think?

[chancancode]

Also wondering if you have been hitting #18 in your real world usage, how often it is an issue for you, and if you have any personal preference on how to deal with it –offer something like what @pzuraq suggested as an optional thing that you use when you hit the issue, or always requiring it, etc

[bwbuchanan]

I agree that caching at the decorator level is probably not needed.

I have not hit #18 in practice – cyclical module dependency graphs are a code smell, in my opinion. @pzuraq's closure suggestion resolves it well in this case, however.