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.

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(store): reduce change detection cycles with pending tasks #2280

Merged
merged 1 commit into from
Dec 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion .bundlemonrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
},
{
"path": "./fesm2022/ngxs-store.mjs",
"maxSize": "103kB",
"maxSize": "105kB",
"maxPercentIncrease": 0.5
}
],
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ $ npm install @ngxs/store@dev
### To become next patch version

- Fix(store): Add root store initializer guard [#2278](https://github.com/ngxs/store/pull/2278)
- Fix(store): Reduce change detection cycles with pending tasks [#2280](https://github.com/ngxs/store/pull/2280)
- Fix(store): Complete action results on destroy [#2282](https://github.com/ngxs/store/pull/2282)
- Refactor(form-plugin): Replace `takeUntil` with `takeUntilDestroyed` [#2283](https://github.com/ngxs/store/pull/2283)

Expand Down
68 changes: 53 additions & 15 deletions packages/store/src/pending-tasks.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { inject, PendingTasks } from '@angular/core';
import { DestroyRef, inject, PendingTasks } from '@angular/core';
import { buffer, debounceTime, filter } from 'rxjs';

import { Actions, ActionStatus } from './actions-stream';
import { withNgxsPreboot } from './standalone-features/preboot';
Expand All @@ -13,22 +14,59 @@ import { withNgxsPreboot } from './standalone-features/preboot';
*/
export function withNgxsPendingTasks() {
return withNgxsPreboot(() => {
const pendingTasks = inject(PendingTasks);
const actions$ = inject(Actions);
const destroyRef = inject(DestroyRef);
const pendingTasks = inject(PendingTasks);

// Removing a pending task via the public API forces a scheduled tick, ensuring that
// stability is async and delayed until there was at least an opportunity to run
// app synchronization.
// Adding a new task every time an action is dispatched drastically increases the
// number of change detection cycles because removing a task schedules a new change
// detection cycle.
// If 10 actions are dispatched with synchronous action handlers, this would trigger
// 10 change detection cycles in a row, potentially leading to an
// `INFINITE_CHANGE_DETECTION` error.
let removeTask: VoidFunction | null = null;

const executedActions = new Set<unknown>();

// If the app is forcely destroyed before all actions are completed,
// we clean up the set of actions being executed to prevent memory leaks
// and remove the pending task to stabilize the app.
destroyRef.onDestroy(() => executedActions.clear());

actions$
.pipe(
filter(context => {
if (context.status === ActionStatus.Dispatched) {
executedActions.add(context.action);
removeTask ||= pendingTasks.add();
return false;
} else {
return true;
}
}),
// Every time an action is completed, we debounce the stream to ensure only one
// task is removed, even if multiple synchronous actions are completed in a row.
// We use `buffer` to collect action contexts because, if we only use
// `debounceTime(0)`, we may lose action contexts that are never removed from the set.
buffer(actions$.pipe(debounceTime(0)))
)
.subscribe(contexts => {
for (const context of contexts) {
if (!executedActions.has(context.action)) {
continue;
}

executedActions.delete(context.action);

const actionToRemoveTaskFnMap = new Map<any, () => void>();

actions$.subscribe(ctx => {
if (ctx.status === ActionStatus.Dispatched) {
const removeTaskFn = pendingTasks.add();
actionToRemoveTaskFnMap.set(ctx.action, removeTaskFn);
} else {
const removeTaskFn = actionToRemoveTaskFnMap.get(ctx.action);
if (typeof removeTaskFn === 'function') {
actionToRemoveTaskFnMap.delete(ctx.action);
removeTaskFn();
// Mark app as stable once all of the debounced actions have completed.
if (executedActions.size === 0) {
removeTask?.();
removeTask = null;
}
}
}
});
});
});
}
19 changes: 13 additions & 6 deletions packages/store/tests/issues/pending-task-notifications.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { bootstrapApplication } from '@angular/platform-browser';
import {
Action,
provideStore,
Selector,
State,
StateContext,
Store,
Expand All @@ -21,6 +22,11 @@ describe('Pending task notifications', () => {
})
@Injectable()
class CounterState {
@Selector()
static getCounter(state: number) {
return state;
}

@Action(Increment)
increment(ctx: StateContext<number>) {
ctx.setState(counter => counter + 1);
Expand All @@ -29,11 +35,12 @@ describe('Pending task notifications', () => {

@Component({
selector: 'app-root',
template: ''
template: '<h1>{{ counter() }}</h1>'
})
class TestComponent {
constructor() {
const store = inject(Store);
readonly counter = this.store.selectSignal(CounterState.getCounter);

constructor(private store: Store) {
for (let i = 0; i < 20; i++) {
store.dispatch(new Increment());
}
Expand All @@ -45,16 +52,16 @@ describe('Pending task notifications', () => {
};

it(
'should call `scheduler.notify()` more than 20 times',
'should NOT call `scheduler.notify()` more than 20 times (because of 20 actions)',
freshPlatform(async () => {
// Arrange
const notifySpy = jest.spyOn(ɵChangeDetectionSchedulerImpl.prototype, 'notify');
// Act
await skipConsoleLogging(() => bootstrapApplication(TestComponent, appConfig));
try {
// Assert
// `notify` has been called more than 20 times because we dispatched 20 times.
expect(notifySpy.mock.calls.length > 20).toBeTruthy();
expect(notifySpy).toHaveBeenCalledTimes(2);
expect(document.body.innerHTML).toContain('<h1>20</h1>');
} finally {
notifySpy.mockRestore();
}
Expand Down
Loading