Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(effects-directive): unregister all effects of a provider #73

Merged
merged 2 commits into from
Jul 1, 2024
Merged

fix(effects-directive): unregister all effects of a provider #73

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12bd426
fix(effects-directive): unregister all effects of a provider
ntziolis Jul 1, 2024
56ddefb
feat(effects-directive): extended test cases
ntziolis Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 16 additions & 3 deletions libs/effects-ng/src/lib/use-directive-effects.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,20 @@ import { provideEffects } from './provide-effects';

const spy = jest.fn();
const spy2 = jest.fn();
const spy3 = jest.fn();
const loadTodos = createAction('[Todos] Load Todos');
const loadTodos2 = createAction('[Todos] Load Todos 2');
const loadTodos3 = createAction('[Todos] Load Todos 3');

@Injectable()
class EffectsOne {
loadTodos$ = createEffect((actions) =>
actions.pipe(ofType(loadTodos), tap(spy))
);

loadTodos3$ = createEffect((actions) =>
actions.pipe(ofType(loadTodos3), tap(spy3))
);
}

@Injectable()
Expand Down Expand Up @@ -54,6 +60,7 @@ describe('provideDirectiveEffects & EffectsDirective', () => {
beforeEach(() => {
spy.mockClear();
spy2.mockClear();
spy3.mockClear();
});

it('should provide effects', () => {
Expand Down Expand Up @@ -95,6 +102,8 @@ describe('provideDirectiveEffects & EffectsDirective', () => {
TestBed.createComponent(componentType);

expect(spy).toHaveBeenCalled();
expect(spy2).not.toHaveBeenCalled();
expect(spy3).not.toHaveBeenCalled();
});

it('should subscribe on the same effects only once', () => {
Expand All @@ -116,6 +125,7 @@ describe('provideDirectiveEffects & EffectsDirective', () => {

expect(spy).toHaveBeenCalledTimes(2);
expect(spy2).toHaveBeenCalledTimes(1);
expect(spy3).not.toHaveBeenCalled();
});

it('should unsubscribe only from effects that was registered via provideDirectiveEffects when component is destroyed', () => {
Expand Down Expand Up @@ -148,25 +158,28 @@ describe('provideDirectiveEffects & EffectsDirective', () => {
const fixture3 = TestBed.createComponent(TestComponent);
const actions = TestBed.inject(Actions);

actions.dispatch(loadTodos2());
actions.dispatch(loadTodos2(), loadTodos3());

expect(spy).toHaveBeenCalledTimes(3);
expect(spy2).toHaveBeenCalledTimes(1);
expect(spy2).toHaveBeenCalledTimes(1);

fixture.destroy();
fixture2.destroy();

actions.dispatch(loadTodos(), loadTodos2());
actions.dispatch(loadTodos(), loadTodos2(), loadTodos3());

expect(spy).toHaveBeenCalledTimes(4);
expect(spy2).toHaveBeenCalledTimes(2);
expect(spy3).toHaveBeenCalledTimes(2);

fixture3.destroy();

actions.dispatch(loadTodos(), loadTodos2());
actions.dispatch(loadTodos(), loadTodos2(), loadTodos3());

expect(spy).toHaveBeenCalledTimes(4);
expect(spy2).toHaveBeenCalledTimes(3);
expect(spy3).toHaveBeenCalledTimes(2);
});

it('should properly determine source instances and manage their effects', () => {
Expand Down
26 changes: 19 additions & 7 deletions libs/effects-ng/src/lib/use-directive-effects.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export class EffectsDirective implements OnDestroy {
private readonly manager = inject(EFFECTS_MANAGER, { optional: true });
private readonly sourceInstancesWithProvidersEffectsTokens = new Map<
any,
ProvidedEffectToken
Array<ProvidedEffectToken>
>();

constructor() {
Expand All @@ -67,9 +67,19 @@ export class EffectsDirective implements OnDestroy {
const sourceInstance = Object.getPrototypeOf(instance);
const token = generateProvidedEffectToken(provider, key);

const tokens = this.sourceInstancesWithProvidersEffectsTokens.has(
sourceInstance
)
? (this.sourceInstancesWithProvidersEffectsTokens.get(
sourceInstance
) as Array<ProvidedEffectToken>)
: [];

tokens.push(token);

this.sourceInstancesWithProvidersEffectsTokens.set(
Object.getPrototypeOf(instance),
token
tokens
);

if (isEffectProvided(sourceInstance, token)) {
Expand All @@ -94,13 +104,15 @@ export class EffectsDirective implements OnDestroy {
private unregisterEffect(): void {
const effects = [
...this.sourceInstancesWithProvidersEffectsTokens.entries(),
].reduce<Effect[]>((effects, [sourceInstance, token]) => {
const effect = getProvidedEffect(sourceInstance, token);
].reduce<Effect[]>((effects, [sourceInstance, tokens]) => {
for (const token of tokens) {
const effect = getProvidedEffect(sourceInstance, token);

decreaseProvidedEffectSources(sourceInstance, token);
decreaseProvidedEffectSources(sourceInstance, token);

if (effect && !isEffectProvided(sourceInstance, token)) {
effects.push(effect);
if (effect && !isEffectProvided(sourceInstance, token)) {
effects.push(effect);
}
}

return effects;
Expand Down
Loading