-
Notifications
You must be signed in to change notification settings - Fork 44
fix: devtools-syncer missing case for extension #226
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
Changes from all commits
5bb5dd8
ff508ca
eda1e4d
15cf4fa
cdbbad2
cf8e583
02e173c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,7 +8,7 @@ import { DevtoolsInnerOptions } from './devtools-feature'; | |
| import { Connection, StoreRegistry, Tracker } from './models'; | ||
|
|
||
| const dummyConnection: Connection = { | ||
| send: () => void true, | ||
| send: () => true, | ||
| }; | ||
|
|
||
| /** | ||
|
|
@@ -30,7 +30,7 @@ export class DevtoolsSyncer implements OnDestroy { | |
| */ | ||
| #stores: StoreRegistry = {}; | ||
| readonly #isBrowser = isPlatformBrowser(inject(PLATFORM_ID)); | ||
| readonly #trackers = [] as Tracker[]; | ||
| readonly #trackers: Tracker[] = []; | ||
| readonly #devtoolsConfig = { | ||
| name: 'NgRx SignalStore', | ||
| ...inject(REDUX_DEVTOOLS_CONFIG, { optional: true }), | ||
|
|
@@ -54,21 +54,47 @@ export class DevtoolsSyncer implements OnDestroy { | |
| #currentId = 1; | ||
|
|
||
| readonly #connection: Connection = this.#isBrowser | ||
| ? window.__REDUX_DEVTOOLS_EXTENSION__ | ||
| ? window.__REDUX_DEVTOOLS_EXTENSION__.connect(this.#devtoolsConfig) | ||
| : dummyConnection | ||
| ? this.#initDevtoolsConnection() | ||
| : dummyConnection; | ||
|
|
||
| constructor() { | ||
| if (!this.#isBrowser) { | ||
| return; | ||
| console.warn( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No warnings if we are on the server. We had |
||
| '[DevtoolsSyncer] Not running in browser. DevTools disabled.', | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| ngOnDestroy(): void { | ||
| currentActionNames.clear(); | ||
| } | ||
|
|
||
| #initDevtoolsConnection(): Connection { | ||
| const extension = window.__REDUX_DEVTOOLS_EXTENSION__; | ||
| if (!extension) { | ||
| console.warn('[DevtoolsSyncer] Redux DevTools extension not found.'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think it can be annoying to throw warnings if DevTools are not |
||
| return dummyConnection; | ||
| } | ||
|
|
||
| try { | ||
| if (typeof extension.connect === 'function') { | ||
| return extension.connect(this.#devtoolsConfig); | ||
| } else { | ||
| console.warn( | ||
| '[DevtoolsSyncer] Redux DevTools extension does not support .connect()', | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.error( | ||
| '[DevtoolsSyncer] Error connecting to Redux DevTools:', | ||
| error, | ||
| ); | ||
| return dummyConnection; | ||
| } | ||
|
|
||
| return dummyConnection; | ||
| } | ||
|
|
||
| syncToDevTools(changedStatePerId: Record<string, object>) { | ||
| const mappedChangedStatePerName = Object.entries(changedStatePerId).reduce( | ||
| (acc, [id, store]) => { | ||
|
|
@@ -78,6 +104,7 @@ export class DevtoolsSyncer implements OnDestroy { | |
| }, | ||
| {} as Record<string, object>, | ||
| ); | ||
|
|
||
| this.#currentState = { | ||
| ...this.#currentState, | ||
| ...mappedChangedStatePerName, | ||
|
|
@@ -90,7 +117,7 @@ export class DevtoolsSyncer implements OnDestroy { | |
| this.#connection.send({ type }, this.#currentState); | ||
| } | ||
|
|
||
| getNextId() { | ||
| getNextId(): string { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally speaking, when you implement a fix or so, try We can leave that, but we should have a general discussion if public |
||
| return String(this.#currentId++); | ||
| } | ||
|
|
||
|
|
@@ -109,21 +136,17 @@ export class DevtoolsSyncer implements OnDestroy { | |
| options: DevtoolsInnerOptions, | ||
| ) { | ||
| let storeName = name; | ||
| const names = Object.values(this.#stores).map((store) => store.name); | ||
|
|
||
| if (names.includes(storeName)) { | ||
| // const { options } = throwIfNull( | ||
| // Object.values(this.#stores).find((store) => store.name === storeName) | ||
| // ); | ||
| if (!options.indexNames) { | ||
| throw new Error(`An instance of the store ${storeName} already exists. \ | ||
| const names = Object.values(this.#stores).map((s) => s.name); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
|
||
| if (names.includes(storeName) && !options.indexNames) { | ||
| throw new Error(`An instance of the store ${storeName} already exists. \ | ||
| Enable automatic indexing via withDevTools('${storeName}', { indexNames: true }), or rename it upon instantiation.`); | ||
| } | ||
| } | ||
|
|
||
| for (let i = 1; names.includes(storeName); i++) { | ||
| storeName = `${name}-${i}`; | ||
| } | ||
|
|
||
| this.#stores[id] = { name: storeName, options }; | ||
|
|
||
| const tracker = options.tracker; | ||
|
|
@@ -136,23 +159,20 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true }) | |
| } | ||
|
|
||
| removeStore(id: string) { | ||
| const name = this.#stores[id].name; | ||
| const name = this.#stores[id]?.name; | ||
|
|
||
| this.#stores = Object.entries(this.#stores).reduce( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can discuss conventional vs. use-case-specific naming - both have their For the sake of consistency, please revert to |
||
| (newStore, [storeId, value]) => { | ||
| if (storeId !== id) { | ||
| newStore[storeId] = value; | ||
| } | ||
| return newStore; | ||
| (acc, [storeId, value]) => { | ||
| if (storeId !== id) acc[storeId] = value; | ||
| return acc; | ||
| }, | ||
| {} as StoreRegistry, | ||
| ); | ||
|
|
||
| this.#currentState = Object.entries(this.#currentState).reduce( | ||
| (newState, [storeName, state]) => { | ||
| if (storeName !== name) { | ||
| newState[name] = state; | ||
| } | ||
| return newState; | ||
| (acc, [storeName, state]) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| if (storeName !== name) acc[storeName] = state; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, object>, | ||
| ); | ||
|
|
@@ -163,36 +183,30 @@ Enable automatic indexing via withDevTools('${storeName}', { indexNames: true }) | |
| } | ||
|
|
||
| renameStore(oldName: string, newName: string) { | ||
| const storeNames = Object.values(this.#stores).map((store) => store.name); | ||
| const storeNames = Object.values(this.#stores).map((s) => s.name); | ||
| const id = throwIfNull( | ||
| Object.keys(this.#stores).find((id) => this.#stores[id].name === oldName), | ||
| Object.keys(this.#stores).find( | ||
| (key) => this.#stores[key].name === oldName, | ||
| ), | ||
| ); | ||
|
|
||
| if (storeNames.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 }; | ||
| } else { | ||
| newStore[id] = value; | ||
| } | ||
| return newStore; | ||
| }, | ||
| {} as StoreRegistry, | ||
| ); | ||
| this.#stores = Object.entries(this.#stores).reduce((acc, [key, value]) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| acc[key] = value.name === oldName ? { ...value, name: newName } : value; | ||
| return acc; | ||
| }, {} as StoreRegistry); | ||
|
|
||
| // we don't rename in #currentState but wait for tracker to notify | ||
| // us with a changed state that contains that name. | ||
| this.#currentState = Object.entries(this.#currentState).reduce( | ||
| (newState, [storeName, state]) => { | ||
| if (storeName !== oldName) { | ||
| newState[storeName] = state; | ||
| } | ||
| return newState; | ||
| (acc, [storeName, state]) => { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| if (storeName !== oldName) acc[storeName] = state; | ||
| return acc; | ||
| }, | ||
| {} as Record<string, object>, | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { PLATFORM_ID } from '@angular/core'; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a separate test file. I did look at other tests and noticed deprecated use of
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I ran all of these tests against the current main branch. Issue #213, however, shows an error from We need a test that reproduces the reported error. Other than that, I do think that you don't have to create a separate file. |
||
| import { TestBed } from '@angular/core/testing'; | ||
| import { DevtoolsSyncer } from '../internal/devtools-syncer.service'; | ||
| import { Tracker } from '../internal/models'; | ||
|
|
||
| describe('DevtoolsSyncer integration with Redux DevTools', () => { | ||
| let connectSpy: jest.Mock; | ||
| let sendSpy: jest.Mock; | ||
|
|
||
| beforeEach(() => { | ||
| sendSpy = jest.fn(); | ||
| connectSpy = jest.fn(() => ({ send: sendSpy })); | ||
| (window as any).__REDUX_DEVTOOLS_EXTENSION__ = { connect: connectSpy }; | ||
|
|
||
| TestBed.resetTestingModule(); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need that? |
||
| TestBed.configureTestingModule({ | ||
| providers: [{ provide: PLATFORM_ID, useValue: 'browser' }], | ||
| }); | ||
| }); | ||
|
|
||
| function createTrackerMock(): Tracker { | ||
| const onChangeMock = jest.fn(); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Didn't think about how this will translate to Vitest, but it should be fine.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something like this: (tracker.onChange as ReturnType<typeof vi.fn>).mockImplementation((cb) => {
cb({ [id]: { count: 42 } });
}); |
||
|
|
||
| return { | ||
| onChange: onChangeMock, | ||
| track: jest.fn(), | ||
| removeStore: jest.fn(), | ||
| notifyRenamedStore: jest.fn(), | ||
| get stores() { | ||
| return {}; // Return an empty object or mock stores as needed | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| it('should send valid state and action type to DevTools', () => { | ||
| const syncer = TestBed.inject(DevtoolsSyncer); | ||
| const id = syncer.getNextId(); | ||
| const tracker = createTrackerMock(); | ||
|
|
||
| (tracker.onChange as jest.Mock).mockImplementation((cb) => { | ||
| cb({ [id]: { count: 42 } }); | ||
| }); | ||
|
|
||
| syncer.addStore(id, 'CounterStore', {} as any, { | ||
| map: (s: object) => s, | ||
| tracker, | ||
| indexNames: false, | ||
| }); | ||
|
|
||
| expect(sendSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: 'Store Update' }), | ||
| expect.objectContaining({ CounterStore: { count: 42 } }), | ||
| ); | ||
| }); | ||
|
|
||
| it('should not send empty state or type', () => { | ||
| const syncer = TestBed.inject(DevtoolsSyncer); | ||
| const id = syncer.getNextId(); | ||
| const tracker = createTrackerMock(); | ||
|
|
||
| (tracker.onChange as jest.Mock).mockImplementation((cb) => { | ||
| cb({ [id]: {} }); | ||
| }); | ||
|
|
||
| syncer.addStore(id, 'EmptyStore', {} as any, { | ||
| map: (s: object) => s, | ||
| tracker, | ||
| indexNames: false, | ||
| }); | ||
|
|
||
| const [action, state] = sendSpy.mock.calls[0]; | ||
| expect(action.type).toBe('Store Update'); | ||
| expect(state.EmptyStore).toEqual({}); | ||
| }); | ||
|
|
||
| it('should handle extension absence gracefully', () => { | ||
| delete (window as any).__REDUX_DEVTOOLS_EXTENSION__; | ||
| const warnSpy = jest.spyOn(console, 'warn'); | ||
| TestBed.inject(DevtoolsSyncer); | ||
|
|
||
| expect(warnSpy).toHaveBeenCalledWith( | ||
| '[DevtoolsSyncer] Redux DevTools extension not found.', | ||
| ); | ||
| warnSpy.mockRestore(); | ||
| }); | ||
|
|
||
| it('should not send if not in browser', () => { | ||
| TestBed.resetTestingModule(); | ||
| TestBed.configureTestingModule({ | ||
| providers: [{ provide: PLATFORM_ID, useValue: 'server' }], | ||
| }); | ||
|
|
||
| TestBed.inject(DevtoolsSyncer); | ||
| expect(connectSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did you change the return type?