From e468a0742128b5a28913d15fb68f4d88188cccba Mon Sep 17 00:00:00 2001 From: Murat Sari Date: Sat, 6 Sep 2025 16:35:20 +0200 Subject: [PATCH] fix: issuses in renameDevtoolsName #213 --- apps/demo/src/app/devtools/todo-store.ts | 9 ++++- .../internal/devtools-syncer.service.ts | 36 +++++++++++------ .../src/lib/devtools/tests/naming.spec.ts | 40 ++++++++++++++++--- .../src/lib/devtools/with-devtools.ts | 2 +- 4 files changed, 67 insertions(+), 20 deletions(-) diff --git a/apps/demo/src/app/devtools/todo-store.ts b/apps/demo/src/app/devtools/todo-store.ts index 7c0685e4..63a2f543 100644 --- a/apps/demo/src/app/devtools/todo-store.ts +++ b/apps/demo/src/app/devtools/todo-store.ts @@ -30,7 +30,14 @@ export const TodoStore = signalStore( }, remove(id: number) { - updateState(store, 'remove todo', removeEntity(id)); + updateState( + store, + 'remove todo', + removeEntity(id), + ({ selectedIds }) => ({ + selectedIds: selectedIds.filter((selectedId) => selectedId !== id), + }), + ); }, toggleFinished(id: number): void { diff --git a/libs/ngrx-toolkit/src/lib/devtools/internal/devtools-syncer.service.ts b/libs/ngrx-toolkit/src/lib/devtools/internal/devtools-syncer.service.ts index a4a67b4a..31f49401 100644 --- a/libs/ngrx-toolkit/src/lib/devtools/internal/devtools-syncer.service.ts +++ b/libs/ngrx-toolkit/src/lib/devtools/internal/devtools-syncer.service.ts @@ -1,7 +1,6 @@ import { isPlatformBrowser } from '@angular/common'; import { inject, Injectable, OnDestroy, PLATFORM_ID } from '@angular/core'; import { StateSource } from '@ngrx/signals'; -import { throwIfNull } from '../../shared/throw-if-null'; import { REDUX_DEVTOOLS_CONFIG } from '../provide-devtools-config'; import { currentActionNames } from './current-action-names'; import { DevtoolsInnerOptions } from './devtools-feature'; @@ -150,7 +149,7 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true }) this.#currentState = Object.entries(this.#currentState).reduce( (newState, [storeName, state]) => { if (storeName !== name) { - newState[name] = state; + newState[storeName] = state; } return newState; }, @@ -162,23 +161,36 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true }) } } - renameStore(oldName: string, newName: string) { - const storeNames = Object.values(this.#stores).map((store) => store.name); - const id = throwIfNull( - Object.keys(this.#stores).find((id) => this.#stores[id].name === oldName), - ); - if (storeNames.includes(newName)) { + /** + * Renames a store identified by its internal id. If the store has already + * been removed (e.g. due to component destruction), this is a no-op. + */ + renameStore(id: string, newName: string) { + const storeEntry = this.#stores[id]; + if (!storeEntry) { + return; + } + const oldName = storeEntry.name; + + if (oldName === newName) { + return; + } + + const otherStoreNames = Object.entries(this.#stores) + .filter(([entryId]) => entryId !== id) + .map(([, s]) => s.name); + if (otherStoreNames.includes(newName)) { throw new Error( `NgRx Toolkit/DevTools: cannot rename from ${oldName} to ${newName}. ${newName} is already assigned to another SignalStore instance.`, ); } this.#stores = Object.entries(this.#stores).reduce( - (newStore, [id, value]) => { - if (value.name === oldName) { - newStore[id] = { ...value, name: newName }; + (newStore, [entryId, value]) => { + if (entryId === id) { + newStore[entryId] = { ...value, name: newName }; } else { - newStore[id] = value; + newStore[entryId] = value; } return newStore; }, diff --git a/libs/ngrx-toolkit/src/lib/devtools/tests/naming.spec.ts b/libs/ngrx-toolkit/src/lib/devtools/tests/naming.spec.ts index 48925f1f..6deff735 100644 --- a/libs/ngrx-toolkit/src/lib/devtools/tests/naming.spec.ts +++ b/libs/ngrx-toolkit/src/lib/devtools/tests/naming.spec.ts @@ -28,7 +28,7 @@ describe('withDevtools / renaming', () => { TestBed.inject(Store); runInInjectionContext(childContext, () => inject(Store)); - TestBed.flushEffects(); + TestBed.tick(); expect(sendSpy).toHaveBeenLastCalledWith( { type: 'Store Update' }, @@ -51,7 +51,7 @@ describe('withDevtools / renaming', () => { const childContext2 = createEnvironmentInjector([Store], envInjector); runInInjectionContext(childContext1, () => inject(Store)); - TestBed.flushEffects(); + TestBed.tick(); childContext1.destroy(); expect(sendSpy.mock.calls).toEqual([ @@ -64,7 +64,7 @@ describe('withDevtools / renaming', () => { ]); runInInjectionContext(childContext2, () => inject(Store)); - TestBed.flushEffects(); + TestBed.tick(); expect(sendSpy.mock.calls).toEqual([ [ { type: 'Store Update' }, @@ -113,7 +113,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or signalStore({ providedIn: 'root' }, withDevtools('flights')), ); - TestBed.flushEffects(); + TestBed.tick(); expect(sendSpy.mock.calls).toEqual([ [ { type: 'Store Update' }, @@ -169,7 +169,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or const store = TestBed.inject(Store); renameDevtoolsName(store, 'flights'); - TestBed.flushEffects(); + TestBed.tick(); expect(sendSpy).toHaveBeenCalledWith( { type: 'Store Update' }, @@ -192,7 +192,7 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or ); TestBed.inject(Store1); const store = TestBed.inject(Store2); - TestBed.flushEffects(); + TestBed.tick(); expect(() => renameDevtoolsName(store, 'shop')).toThrow( 'NgRx Toolkit/DevTools: cannot rename from mall to shop. shop is already assigned to another SignalStore instance.', @@ -212,5 +212,33 @@ Enable automatic indexing via withDevTools('flights', { indexNames: true }), or "Devtools extensions haven't been added to this store.", ); }); + + it('should ignore rename after the store has been destroyed', () => { + const { sendSpy } = setupExtensions(); + + const Store = signalStore( + withDevtools('flight'), + withState({ name: 'Product', price: 10.5 }), + ); + + const childContext = createEnvironmentInjector( + [Store], + TestBed.inject(EnvironmentInjector), + ); + + const store = childContext.get(Store); + TestBed.tick(); + + expect(sendSpy).toHaveBeenCalledWith( + { type: 'Store Update' }, + { flight: { name: 'Product', price: 10.5 } }, + ); + + childContext.destroy(); + TestBed.tick(); + + // Previously this could throw; now it is a no-op + expect(() => renameDevtoolsName(store, 'flights')).not.toThrow(); + }); }); }); diff --git a/libs/ngrx-toolkit/src/lib/devtools/with-devtools.ts b/libs/ngrx-toolkit/src/lib/devtools/with-devtools.ts index 5827e7ae..71e8aaab 100644 --- a/libs/ngrx-toolkit/src/lib/devtools/with-devtools.ts +++ b/libs/ngrx-toolkit/src/lib/devtools/with-devtools.ts @@ -51,7 +51,7 @@ export function withDevtools(name: string, ...features: DevtoolsFeature[]) { // TODO: use withProps and symbols return { [renameDevtoolsMethodName]: (newName: string) => { - syncer.renameStore(name, newName); + syncer.renameStore(id, newName); }, [uniqueDevtoolsId]: () => id, } as Record unknown>;