From 7a94b55da17dde3d7992276123f629d16b602d77 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sat, 9 May 2020 16:45:10 +0200 Subject: [PATCH 1/5] feat(store): add runtime check for action type uniqueness --- modules/store/spec/runtime_checks.spec.ts | 60 ++++++++++++++++++++++- modules/store/src/action_creator.ts | 6 +++ modules/store/src/globals.ts | 1 + modules/store/src/models.ts | 5 ++ modules/store/src/runtime_checks.ts | 34 +++++++++++++ modules/store/src/store_module.ts | 19 +++++-- modules/store/src/tokens.ts | 4 ++ 7 files changed, 125 insertions(+), 4 deletions(-) create mode 100644 modules/store/src/globals.ts diff --git a/modules/store/spec/runtime_checks.spec.ts b/modules/store/spec/runtime_checks.spec.ts index f0e6f581a7..a9bfc202f7 100644 --- a/modules/store/spec/runtime_checks.spec.ts +++ b/modules/store/spec/runtime_checks.spec.ts @@ -1,9 +1,16 @@ import * as ngCore from '@angular/core'; import { TestBed, fakeAsync, flush } from '@angular/core/testing'; -import { Store, StoreModule, META_REDUCERS, USER_RUNTIME_CHECKS } from '..'; +import { + Store, + StoreModule, + META_REDUCERS, + USER_RUNTIME_CHECKS, + createAction, +} from '..'; import { createActiveRuntimeChecks } from '../src/runtime_checks'; import { RuntimeChecks, Action } from '../src/models'; import * as metaReducers from '../src/meta-reducers'; +import { REGISTERED_ACTION_TYPES } from '../src/globals'; describe('Runtime checks:', () => { describe('createActiveRuntimeChecks:', () => { @@ -14,6 +21,7 @@ describe('Runtime checks:', () => { strictActionImmutability: true, strictStateImmutability: true, strictActionWithinNgZone: false, + strictActionTypeUniqueness: false, }); }); @@ -25,6 +33,7 @@ describe('Runtime checks:', () => { strictActionImmutability: false, strictStateImmutability: false, strictActionWithinNgZone: true, + strictActionTypeUniqueness: true, }) ).toEqual({ strictStateSerializability: true, @@ -32,6 +41,7 @@ describe('Runtime checks:', () => { strictActionImmutability: false, strictStateImmutability: false, strictActionWithinNgZone: true, + strictActionTypeUniqueness: true, }); }); @@ -44,6 +54,7 @@ describe('Runtime checks:', () => { strictActionImmutability: false, strictStateImmutability: false, strictActionWithinNgZone: false, + strictActionTypeUniqueness: false, }); }); @@ -55,6 +66,7 @@ describe('Runtime checks:', () => { strictStateSerializability: true, strictActionSerializability: true, strictActionWithinNgZone: true, + strictActionTypeUniqueness: true, }) ).toEqual({ strictStateSerializability: false, @@ -62,6 +74,7 @@ describe('Runtime checks:', () => { strictActionImmutability: false, strictStateImmutability: false, strictActionWithinNgZone: false, + strictActionTypeUniqueness: false, }); }); }); @@ -384,6 +397,51 @@ describe('Runtime checks:', () => { }); }); +describe('ActionType uniqueness', () => { + beforeEach(() => { + REGISTERED_ACTION_TYPES.length = 0; + }); + + it('should throw when having no duplicate action types', () => { + createAction('action 1'); + createAction('action 1'); + + expect(() => { + const store = setupStore({ strictActionTypeUniqueness: true }); + }).toThrowError(/Action types are registered more than once/); + }); + + it('should not throw when having no duplicate action types', () => { + createAction('action 1'); + createAction('action 2'); + + expect(() => { + const store = setupStore({ strictActionTypeUniqueness: true }); + }).not.toThrowError(); + }); + + it('should not register action types if devMode is false', () => { + spyOn(ngCore, 'isDevMode').and.returnValue(false); + + createAction('action 1'); + createAction('action 1'); + + expect(REGISTERED_ACTION_TYPES.length).toBe(0); + + expect(() => { + const store = setupStore({ strictActionTypeUniqueness: false }); + }).not.toThrowError(); + }); + + it('should not throw when disabled', () => { + createAction('action 1'); + createAction('action 1'); + expect(() => { + const store = setupStore({ strictActionTypeUniqueness: false }); + }).not.toThrowError(); + }); +}); + function setupStore(runtimeChecks?: Partial): Store { TestBed.configureTestingModule({ imports: [ diff --git a/modules/store/src/action_creator.ts b/modules/store/src/action_creator.ts index 341bb5b79b..851a5fa18c 100644 --- a/modules/store/src/action_creator.ts +++ b/modules/store/src/action_creator.ts @@ -6,6 +6,8 @@ import { NotAllowedCheck, Props, } from './models'; +import { isDevMode } from '@angular/core'; +import { REGISTERED_ACTION_TYPES } from './globals'; // Action creators taken from ts-action library and modified a bit to better // fit current NgRx usage. Thank you Nicholas Jamieson (@cartant). @@ -101,6 +103,10 @@ export function createAction( type: T, config?: { _as: 'props' } | C ): ActionCreator { + if (isDevMode()) { + REGISTERED_ACTION_TYPES.push(type); + } + if (typeof config === 'function') { return defineType(type, (...args: any[]) => ({ ...config(...args), diff --git a/modules/store/src/globals.ts b/modules/store/src/globals.ts new file mode 100644 index 0000000000..37ae7e5af4 --- /dev/null +++ b/modules/store/src/globals.ts @@ -0,0 +1 @@ +export let REGISTERED_ACTION_TYPES: string[] = []; diff --git a/modules/store/src/models.ts b/modules/store/src/models.ts index 84eb35f5ef..d2f80c163f 100644 --- a/modules/store/src/models.ts +++ b/modules/store/src/models.ts @@ -120,4 +120,9 @@ export interface RuntimeChecks { * Verifies that actions are dispatched within NgZone */ strictActionWithinNgZone: boolean; + + /** + * Verifies that action types are not registered more than once + */ + strictActionTypeUniqueness: boolean; } diff --git a/modules/store/src/runtime_checks.ts b/modules/store/src/runtime_checks.ts index 55c516fa34..87b867be37 100644 --- a/modules/store/src/runtime_checks.ts +++ b/modules/store/src/runtime_checks.ts @@ -10,7 +10,10 @@ import { _ACTIVE_RUNTIME_CHECKS, META_REDUCERS, USER_RUNTIME_CHECKS, + _ACTION_TYPE_UNIQUENESS_CHECK, } from './tokens'; +import { REGISTERED_ACTION_TYPES } from './globals'; +import { RUNTIME_CHECK_URL } from './meta-reducers/utils'; export function createActiveRuntimeChecks( runtimeChecks?: Partial @@ -22,6 +25,7 @@ export function createActiveRuntimeChecks( strictStateImmutability: true, strictActionImmutability: true, strictActionWithinNgZone: false, + strictActionTypeUniqueness: false, ...runtimeChecks, }; } @@ -32,6 +36,7 @@ export function createActiveRuntimeChecks( strictStateImmutability: false, strictActionImmutability: false, strictActionWithinNgZone: false, + strictActionTypeUniqueness: false, }; } @@ -118,8 +123,37 @@ export function provideRuntimeChecks( ]; } +export function checkForActionTypeUniqueness(): Provider[] { + return [ + { + provide: _ACTION_TYPE_UNIQUENESS_CHECK, + multi: true, + deps: [_ACTIVE_RUNTIME_CHECKS], + useFactory: _actionTypeUniquenessCheck, + }, + ]; +} + export function _runtimeChecksFactory( runtimeChecks: RuntimeChecks ): RuntimeChecks { return runtimeChecks; } + +export function _actionTypeUniquenessCheck(config: RuntimeChecks) { + if (!config.strictActionTypeUniqueness) { + return; + } + + const duplicates = REGISTERED_ACTION_TYPES.filter( + (type, index, arr) => arr.indexOf(type) !== index + ); + + if (duplicates.length) { + throw new Error( + `Action types are registered more than once, ${duplicates + .map(type => `"${type}"`) + .join(', ')}. ${RUNTIME_CHECK_URL}#strictactiontypeuniqueness` + ); + } +} diff --git a/modules/store/src/store_module.ts b/modules/store/src/store_module.ts index 19ecc745fd..72c61180d1 100644 --- a/modules/store/src/store_module.ts +++ b/modules/store/src/store_module.ts @@ -37,6 +37,8 @@ import { USER_PROVIDED_META_REDUCERS, _RESOLVED_META_REDUCERS, _ROOT_STORE_GUARD, + _ACTIVE_RUNTIME_CHECKS, + _ACTION_TYPE_UNIQUENESS_CHECK, } from './tokens'; import { ACTIONS_SUBJECT_PROVIDERS, ActionsSubject } from './actions_subject'; import { @@ -50,7 +52,10 @@ import { } from './scanned_actions_subject'; import { STATE_PROVIDERS } from './state'; import { STORE_PROVIDERS, Store } from './store'; -import { provideRuntimeChecks } from './runtime_checks'; +import { + provideRuntimeChecks, + checkForActionTypeUniqueness, +} from './runtime_checks'; @NgModule({}) export class StoreRootModule { @@ -61,7 +66,10 @@ export class StoreRootModule { store: Store, @Optional() @Inject(_ROOT_STORE_GUARD) - guard: any + guard: any, + @Optional() + @Inject(_ACTION_TYPE_UNIQUENESS_CHECK) + actionCheck: any ) {} } @@ -71,7 +79,10 @@ export class StoreFeatureModule implements OnDestroy { @Inject(_STORE_FEATURES) private features: StoreFeature[], @Inject(FEATURE_REDUCERS) private featureReducers: ActionReducerMap[], private reducerManager: ReducerManager, - root: StoreRootModule + root: StoreRootModule, + @Optional() + @Inject(_ACTION_TYPE_UNIQUENESS_CHECK) + actionCheck: any ) { const feats = features.map((feature, index) => { const featureReducerCollection = featureReducers.shift(); @@ -166,6 +177,7 @@ export class StoreModule { STATE_PROVIDERS, STORE_PROVIDERS, provideRuntimeChecks(config.runtimeChecks), + checkForActionTypeUniqueness(), ], }; } @@ -238,6 +250,7 @@ export class StoreModule { ], useFactory: _createFeatureReducers, }, + checkForActionTypeUniqueness(), ], }; } diff --git a/modules/store/src/tokens.ts b/modules/store/src/tokens.ts index d0d6e1861d..224849bed7 100644 --- a/modules/store/src/tokens.ts +++ b/modules/store/src/tokens.ts @@ -86,3 +86,7 @@ export const _USER_RUNTIME_CHECKS = new InjectionToken( export const _ACTIVE_RUNTIME_CHECKS = new InjectionToken( '@ngrx/store Internal Runtime Checks' ); + +export const _ACTION_TYPE_UNIQUENESS_CHECK = new InjectionToken( + '@ngrx/store Check if Action types are unique' +); From ff90e211e3e6a5fa493ecc3930d67220a3d8ce6b Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Wed, 13 May 2020 19:32:02 +0200 Subject: [PATCH 2/5] refactor(store): REGISTERED_ACTION_TYPES as dictionary --- modules/effects/spec/types/utils.ts | 2 +- modules/router-store/spec/types/utils.ts | 2 +- modules/store/spec/runtime_checks.spec.ts | 18 +++++++++++------- modules/store/spec/types/utils.ts | 2 +- modules/store/src/action_creator.ts | 2 +- modules/store/src/globals.ts | 6 +++++- modules/store/src/runtime_checks.ts | 6 +++--- 7 files changed, 23 insertions(+), 15 deletions(-) diff --git a/modules/effects/spec/types/utils.ts b/modules/effects/spec/types/utils.ts index 08232a9458..e162b04990 100644 --- a/modules/effects/spec/types/utils.ts +++ b/modules/effects/spec/types/utils.ts @@ -1,6 +1,6 @@ export const compilerOptions = () => ({ moduleResolution: 'node', - target: 'es2015', + target: 'es2017', baseUrl: '.', experimentalDecorators: true, paths: { diff --git a/modules/router-store/spec/types/utils.ts b/modules/router-store/spec/types/utils.ts index 407985e92d..39ed018ea8 100644 --- a/modules/router-store/spec/types/utils.ts +++ b/modules/router-store/spec/types/utils.ts @@ -1,6 +1,6 @@ export const compilerOptions = () => ({ moduleResolution: 'node', - target: 'es2015', + target: 'es2017', baseUrl: '.', experimentalDecorators: true, paths: { diff --git a/modules/store/spec/runtime_checks.spec.ts b/modules/store/spec/runtime_checks.spec.ts index a9bfc202f7..254ef5cdd1 100644 --- a/modules/store/spec/runtime_checks.spec.ts +++ b/modules/store/spec/runtime_checks.spec.ts @@ -10,7 +10,10 @@ import { import { createActiveRuntimeChecks } from '../src/runtime_checks'; import { RuntimeChecks, Action } from '../src/models'; import * as metaReducers from '../src/meta-reducers'; -import { REGISTERED_ACTION_TYPES } from '../src/globals'; +import { + REGISTERED_ACTION_TYPES, + resetRegisteredActionTypes, +} from '../src/globals'; describe('Runtime checks:', () => { describe('createActiveRuntimeChecks:', () => { @@ -399,7 +402,7 @@ describe('Runtime checks:', () => { describe('ActionType uniqueness', () => { beforeEach(() => { - REGISTERED_ACTION_TYPES.length = 0; + resetRegisteredActionTypes(); }); it('should throw when having no duplicate action types', () => { @@ -407,7 +410,7 @@ describe('ActionType uniqueness', () => { createAction('action 1'); expect(() => { - const store = setupStore({ strictActionTypeUniqueness: true }); + setupStore({ strictActionTypeUniqueness: true }); }).toThrowError(/Action types are registered more than once/); }); @@ -416,7 +419,7 @@ describe('ActionType uniqueness', () => { createAction('action 2'); expect(() => { - const store = setupStore({ strictActionTypeUniqueness: true }); + setupStore({ strictActionTypeUniqueness: true }); }).not.toThrowError(); }); @@ -426,18 +429,19 @@ describe('ActionType uniqueness', () => { createAction('action 1'); createAction('action 1'); - expect(REGISTERED_ACTION_TYPES.length).toBe(0); + expect(REGISTERED_ACTION_TYPES).toEqual({}); expect(() => { - const store = setupStore({ strictActionTypeUniqueness: false }); + setupStore({ strictActionTypeUniqueness: false }); }).not.toThrowError(); }); it('should not throw when disabled', () => { createAction('action 1'); createAction('action 1'); + expect(() => { - const store = setupStore({ strictActionTypeUniqueness: false }); + setupStore({ strictActionTypeUniqueness: false }); }).not.toThrowError(); }); }); diff --git a/modules/store/spec/types/utils.ts b/modules/store/spec/types/utils.ts index 24f2eada6a..63d17eacf0 100644 --- a/modules/store/spec/types/utils.ts +++ b/modules/store/spec/types/utils.ts @@ -1,6 +1,6 @@ export const compilerOptions = () => ({ moduleResolution: 'node', - target: 'es2015', + target: 'es2017', baseUrl: '.', experimentalDecorators: true, paths: { diff --git a/modules/store/src/action_creator.ts b/modules/store/src/action_creator.ts index 851a5fa18c..f634db3b0b 100644 --- a/modules/store/src/action_creator.ts +++ b/modules/store/src/action_creator.ts @@ -104,7 +104,7 @@ export function createAction( config?: { _as: 'props' } | C ): ActionCreator { if (isDevMode()) { - REGISTERED_ACTION_TYPES.push(type); + REGISTERED_ACTION_TYPES[type] = (REGISTERED_ACTION_TYPES[type] || 0) + 1; } if (typeof config === 'function') { diff --git a/modules/store/src/globals.ts b/modules/store/src/globals.ts index 37ae7e5af4..02aa4c6c81 100644 --- a/modules/store/src/globals.ts +++ b/modules/store/src/globals.ts @@ -1 +1,5 @@ -export let REGISTERED_ACTION_TYPES: string[] = []; +export let REGISTERED_ACTION_TYPES: { [actionType: string]: number } = {}; + +export function resetRegisteredActionTypes() { + REGISTERED_ACTION_TYPES = {}; +} diff --git a/modules/store/src/runtime_checks.ts b/modules/store/src/runtime_checks.ts index 87b867be37..882900cfc5 100644 --- a/modules/store/src/runtime_checks.ts +++ b/modules/store/src/runtime_checks.ts @@ -145,9 +145,9 @@ export function _actionTypeUniquenessCheck(config: RuntimeChecks) { return; } - const duplicates = REGISTERED_ACTION_TYPES.filter( - (type, index, arr) => arr.indexOf(type) !== index - ); + const duplicates = Object.entries(REGISTERED_ACTION_TYPES) + .filter(([_type, registrations]) => registrations > 1) + .map(([type]) => type); if (duplicates.length) { throw new Error( From abbea3c085ea33a0e38e8ae5c2bafaaa658da07f Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Wed, 13 May 2020 19:37:09 +0200 Subject: [PATCH 3/5] docs(store): add strictActionTypeUniqueness check --- projects/example-app/src/app/app.module.ts | 1 + .../store/configuration/runtime-checks.md | 63 +++++++++++++------ 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/projects/example-app/src/app/app.module.ts b/projects/example-app/src/app/app.module.ts index d5f7bcbc1c..760b16fd69 100644 --- a/projects/example-app/src/app/app.module.ts +++ b/projects/example-app/src/app/app.module.ts @@ -41,6 +41,7 @@ import { AppComponent } from '@example-app/core/containers'; strictStateSerializability: true, strictActionSerializability: true, strictActionWithinNgZone: true, + strictActionTypeUniqueness: true, }, }), diff --git a/projects/ngrx.io/content/guide/store/configuration/runtime-checks.md b/projects/ngrx.io/content/guide/store/configuration/runtime-checks.md index 9b112e1b17..4411edcbd2 100644 --- a/projects/ngrx.io/content/guide/store/configuration/runtime-checks.md +++ b/projects/ngrx.io/content/guide/store/configuration/runtime-checks.md @@ -11,6 +11,7 @@ Runtime checks are here to guide developers to follow the NgRx and Redux core co - [`strictStateSerializability`](#strictstateserializability): verifies if the state is serializable - [`strictActionSerializability`](#strictactionserializability): verifies if the actions are serializable - [`strictActionWithinNgZone`](#strictactionwithinngzone): verifies if actions are dispatched within NgZone + - [`strictActionTypeUniqueness`](#strictactiontypeuniqueness): verifies if registered action types are unique All checks will automatically be disabled in production builds. @@ -27,7 +28,8 @@ It's possible to override the default configuration of runtime checks. To do so, strictActionImmutability: true, strictStateSerializability: true, strictActionSerializability: true, - strictActionWithinNgZone: true + strictActionWithinNgZone: true, + strictActionTypeUniqueness: true, }, }), ], @@ -38,7 +40,7 @@ export class AppModule {}
The serializability runtime checks cannot be enabled if you use `@ngrx/router-store` with the `DefaultRouterStateSerializer`. The [default serializer](guide/router-store/configuration) has an unserializable router state and actions that are not serializable. To use the serializability runtime checks either use the `MinimalRouterStateSerializer` or implement a custom router state serializer. -This also applies to Ivy with immutability runtime checks. +This also applies to Ivy with immutability runtime checks.
@@ -62,11 +64,12 @@ export const reducer = createReducer(initialState, To fix the above violation, a new reference to the state has to be created: ```ts -export const reducer = createReducer(initialState, +export const reducer = createReducer( + initialState, on(addTodo, (state, { todo }) => ({ ...state, todoInput: '', - todos: [...state.todos, todo] + todos: [...state.todos, todo], })) ); ``` @@ -95,12 +98,13 @@ To fix the above violation, the todo's id should be set in the action creator or ```ts export const addTodo = createAction( '[Todo List] Add Todo', - (description: string) => ({ id: generateUniqueId(), description}) + (description: string) => ({ id: generateUniqueId(), description }) ); -export const reducer = createReducer(initialState, +export const reducer = createReducer( + initialState, on(addTodo, (state, { todo }) => ({ ...state, - todos: [...state.todos, todo] + todos: [...state.todos, todo], })) ); ``` @@ -112,7 +116,8 @@ This check verifies if the state is serializable. A serializable state is import Example violation of the rule: ```ts -export const reducer = createReducer(initialState, +export const reducer = createReducer( + initialState, on(completeTodo, (state, { id }) => ({ ...state, todos: { @@ -122,7 +127,7 @@ export const reducer = createReducer(initialState, // Violation, Date is not serializable completedOn: new Date(), }, - } + }, })) ); ``` @@ -130,16 +135,17 @@ export const reducer = createReducer(initialState, As a fix of the above violation the `Date` object must be made serializable: ```ts -export const reducer = createReducer(initialState, +export const reducer = createReducer( + initialState, on(completeTodo, (state, { id }) => ({ ...state, todos: { ...state.todos, [id]: { ...state.todos[id], - completedOn: new Date().toJSON() - } - } + completedOn: new Date().toJSON(), + }, + }, })) ); ``` @@ -155,17 +161,20 @@ const createTodo = createAction('[Todo List] Add new todo', todo => ({ todo, // Violation, a function is not serializable logTodo: () => { - console.log(todo) + console.log(todo); }, -})) +})); ``` The fix for this violation is to not add functions on actions, as a replacement a function can be created: ```ts -const createTodo = createAction('[Todo List] Add new todo', props<{ todo: Todo }>()); +const createTodo = createAction( + '[Todo List] Add new todo', + props<{ todo: Todo }>() +); -function logTodo (todo: Todo) { +function logTodo(todo: Todo) { console.log(todo); } ``` @@ -184,7 +193,7 @@ Example violation of the rule: ```ts // Callback running outside of NgZone -function callbackOutsideNgZone(){ +function callbackOutsideNgZone() { this.store.dispatch(clearTodos()); } ``` @@ -203,3 +212,21 @@ function callbackOutsideNgZone(){ } } ``` + +## strictActionTypeUniqueness + +The `strictActionTypeUniqueness` guards you against registering the same action type more than once. + +Example violation of the rule: + +```ts +export const customerPageLoaded = createAction('[Customers Page] Loaded'); +export const customerPageRefreshed = createAction('[Customers Page] Loaded'); +``` + +The fix of the violation is to create unique action types: + +```ts +export const customerPageLoaded = createAction('[Customers Page] Loaded'); +export const customerPageRefreshed = createAction('[Customers Page] Refreshed'); +``` From 3d1b87b3eeb6af02be6c443408159b1c3fb1a4c3 Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Wed, 13 May 2020 20:01:46 +0200 Subject: [PATCH 4/5] fix(store): remove the isDev check --- modules/store/spec/runtime_checks.spec.ts | 13 ------------- modules/store/src/action_creator.ts | 5 +---- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/modules/store/spec/runtime_checks.spec.ts b/modules/store/spec/runtime_checks.spec.ts index 254ef5cdd1..4087ea22f8 100644 --- a/modules/store/spec/runtime_checks.spec.ts +++ b/modules/store/spec/runtime_checks.spec.ts @@ -423,19 +423,6 @@ describe('ActionType uniqueness', () => { }).not.toThrowError(); }); - it('should not register action types if devMode is false', () => { - spyOn(ngCore, 'isDevMode').and.returnValue(false); - - createAction('action 1'); - createAction('action 1'); - - expect(REGISTERED_ACTION_TYPES).toEqual({}); - - expect(() => { - setupStore({ strictActionTypeUniqueness: false }); - }).not.toThrowError(); - }); - it('should not throw when disabled', () => { createAction('action 1'); createAction('action 1'); diff --git a/modules/store/src/action_creator.ts b/modules/store/src/action_creator.ts index f634db3b0b..bab2d9c845 100644 --- a/modules/store/src/action_creator.ts +++ b/modules/store/src/action_creator.ts @@ -6,7 +6,6 @@ import { NotAllowedCheck, Props, } from './models'; -import { isDevMode } from '@angular/core'; import { REGISTERED_ACTION_TYPES } from './globals'; // Action creators taken from ts-action library and modified a bit to better @@ -103,9 +102,7 @@ export function createAction( type: T, config?: { _as: 'props' } | C ): ActionCreator { - if (isDevMode()) { - REGISTERED_ACTION_TYPES[type] = (REGISTERED_ACTION_TYPES[type] || 0) + 1; - } + REGISTERED_ACTION_TYPES[type] = (REGISTERED_ACTION_TYPES[type] || 0) + 1; if (typeof config === 'function') { return defineType(type, (...args: any[]) => ({ From 08e7ad796878c66e8203634bd5bc5ef7bcab97cc Mon Sep 17 00:00:00 2001 From: timdeschryver <28659384+timdeschryver@users.noreply.github.com> Date: Wed, 27 May 2020 19:51:26 +0200 Subject: [PATCH 5/5] review changes --- modules/store/spec/runtime_checks.spec.ts | 288 ++++++++++------------ modules/store/src/models.ts | 8 +- modules/store/src/runtime_checks.ts | 18 +- 3 files changed, 139 insertions(+), 175 deletions(-) diff --git a/modules/store/spec/runtime_checks.spec.ts b/modules/store/spec/runtime_checks.spec.ts index 4087ea22f8..7c09045876 100644 --- a/modules/store/spec/runtime_checks.spec.ts +++ b/modules/store/spec/runtime_checks.spec.ts @@ -164,7 +164,7 @@ describe('Runtime checks:', () => { let logs: string[] = []; function metaReducerFactory(logMessage: string) { return function metaReducer(reducer: any) { - return function(state: any, action: any) { + return function (state: any, action: any) { logs.push(logMessage); return reducer(state, action); }; @@ -208,41 +208,32 @@ describe('Runtime checks:', () => { describe('State Serialization:', () => { const invalidAction = () => ({ type: ErrorTypes.UnserializableState }); - it( - 'should throw when enabled', - fakeAsync(() => { - const store = setupStore({ strictStateSerializability: true }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).toThrowError(/Detected unserializable state/); - }) - ); - - it( - 'should not throw when disabled', - fakeAsync(() => { - const store = setupStore({ strictStateSerializability: false }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrow(); - }) - ); - - it( - 'should not throw for NgRx actions', - fakeAsync(() => { - const store = setupStore({ strictStateSerializability: true }); - - expect(() => { - store.dispatch(makeNgrxAction(invalidAction())); - flush(); - }).not.toThrow(); - }) - ); + it('should throw when enabled', fakeAsync(() => { + const store = setupStore({ strictStateSerializability: true }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).toThrowError(/Detected unserializable state/); + })); + + it('should not throw when disabled', fakeAsync(() => { + const store = setupStore({ strictStateSerializability: false }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrow(); + })); + + it('should not throw for NgRx actions', fakeAsync(() => { + const store = setupStore({ strictStateSerializability: true }); + + expect(() => { + store.dispatch(makeNgrxAction(invalidAction())); + flush(); + }).not.toThrow(); + })); }); describe('Action Serialization:', () => { @@ -251,29 +242,23 @@ describe('Runtime checks:', () => { invalid: new Date(), }); - it( - 'should throw when enabled', - fakeAsync(() => { - const store = setupStore({ strictActionSerializability: true }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).toThrowError(/Detected unserializable action/); - }) - ); - - it( - 'should not throw when disabled', - fakeAsync(() => { - const store = setupStore({ strictActionSerializability: false }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrow(); - }) - ); + it('should throw when enabled', fakeAsync(() => { + const store = setupStore({ strictActionSerializability: true }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).toThrowError(/Detected unserializable action/); + })); + + it('should not throw when disabled', fakeAsync(() => { + const store = setupStore({ strictActionSerializability: false }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrow(); + })); }); describe('State Mutations', () => { @@ -281,29 +266,23 @@ describe('Runtime checks:', () => { type: ErrorTypes.MutateState, }); - it( - 'should throw when enabled', - fakeAsync(() => { - const store = setupStore({ strictStateImmutability: true }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).toThrowError(/Cannot add property/); - }) - ); - - it( - 'should not throw when disabled', - fakeAsync(() => { - const store = setupStore({ strictStateImmutability: false }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrow(); - }) - ); + it('should throw when enabled', fakeAsync(() => { + const store = setupStore({ strictStateImmutability: true }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).toThrowError(/Cannot add property/); + })); + + it('should not throw when disabled', fakeAsync(() => { + const store = setupStore({ strictStateImmutability: false }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrow(); + })); }); describe('Action Mutations', () => { @@ -312,100 +291,83 @@ describe('Runtime checks:', () => { foo: 'foo', }); - it( - 'should throw when enabled', - fakeAsync(() => { - const store = setupStore({ strictActionImmutability: true }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).toThrowError(/Cannot assign to read only property/); - }) - ); - - it( - 'should not throw when disabled', - fakeAsync(() => { - const store = setupStore({ strictActionImmutability: false }); - - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrow(); - }) - ); - - it( - 'should not throw for NgRx actions', - fakeAsync(() => { - const store = setupStore({ strictActionImmutability: true }); - - expect(() => { - store.dispatch(makeNgrxAction(invalidAction())); - flush(); - }).not.toThrow(); - }) - ); + it('should throw when enabled', fakeAsync(() => { + const store = setupStore({ strictActionImmutability: true }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).toThrowError(/Cannot assign to read only property/); + })); + + it('should not throw when disabled', fakeAsync(() => { + const store = setupStore({ strictActionImmutability: false }); + + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrow(); + })); + + it('should not throw for NgRx actions', fakeAsync(() => { + const store = setupStore({ strictActionImmutability: true }); + + expect(() => { + store.dispatch(makeNgrxAction(invalidAction())); + flush(); + }).not.toThrow(); + })); }); describe('Action in NgZone', () => { const invalidAction = () => ({ type: ErrorTypes.OutOfNgZoneAction }); - it( - 'should throw when running outside ngZone', - fakeAsync(() => { - ngCore.NgZone.isInAngularZone = jasmine - .createSpy('isInAngularZone') - .and.returnValue(false); - const store = setupStore({ strictActionWithinNgZone: true }); - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).toThrowError( - "Action 'Action triggered outside of NgZone' running outside NgZone. https://ngrx.io/guide/store/configuration/runtime-checks#strictactionwithinngzone" - ); - }) - ); - - it( - 'should not throw when running in ngZone', - fakeAsync(() => { - ngCore.NgZone.isInAngularZone = jasmine - .createSpy('isInAngularZone') - .and.returnValue(true); - const store = setupStore({ strictActionWithinNgZone: true }); - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrowError(); - - expect(ngCore.NgZone.isInAngularZone).toHaveBeenCalled(); - }) - ); - - it( - 'should not be called when disabled', - fakeAsync(() => { - const store = setupStore({ strictActionWithinNgZone: false }); - ngCore.NgZone.isInAngularZone = jasmine.createSpy('isInAngularZone'); - expect(() => { - store.dispatch(invalidAction()); - flush(); - }).not.toThrow(); - - expect(ngCore.NgZone.isInAngularZone).not.toHaveBeenCalled(); - }) - ); + it('should throw when running outside ngZone', fakeAsync(() => { + ngCore.NgZone.isInAngularZone = jasmine + .createSpy('isInAngularZone') + .and.returnValue(false); + const store = setupStore({ strictActionWithinNgZone: true }); + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).toThrowError( + "Action 'Action triggered outside of NgZone' running outside NgZone. https://ngrx.io/guide/store/configuration/runtime-checks#strictactionwithinngzone" + ); + })); + + it('should not throw when running in ngZone', fakeAsync(() => { + ngCore.NgZone.isInAngularZone = jasmine + .createSpy('isInAngularZone') + .and.returnValue(true); + const store = setupStore({ strictActionWithinNgZone: true }); + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrowError(); + + expect(ngCore.NgZone.isInAngularZone).toHaveBeenCalled(); + })); + + it('should not be called when disabled', fakeAsync(() => { + const store = setupStore({ strictActionWithinNgZone: false }); + ngCore.NgZone.isInAngularZone = jasmine.createSpy('isInAngularZone'); + expect(() => { + store.dispatch(invalidAction()); + flush(); + }).not.toThrow(); + + expect(ngCore.NgZone.isInAngularZone).not.toHaveBeenCalled(); + })); }); }); describe('ActionType uniqueness', () => { beforeEach(() => { + // Clear before each test because action types are registered during tests resetRegisteredActionTypes(); }); - it('should throw when having no duplicate action types', () => { + it('should throw when having duplicate action types', () => { createAction('action 1'); createAction('action 1'); diff --git a/modules/store/src/models.ts b/modules/store/src/models.ts index d2f80c163f..f6f279abe4 100644 --- a/modules/store/src/models.ts +++ b/modules/store/src/models.ts @@ -24,7 +24,7 @@ export interface ActionReducer { } export type ActionReducerMap = { - [p in keyof T]: ActionReducer + [p in keyof T]: ActionReducer; }; export interface ActionReducerFactory { @@ -75,7 +75,9 @@ export type Creator< export type NotAllowedCheck = T extends any[] ? ArraysAreNotAllowed - : T extends { type: any } ? TypePropertyIsNotAllowed : unknown; + : T extends { type: any } + ? TypePropertyIsNotAllowed + : unknown; /** * See `Creator`. @@ -124,5 +126,5 @@ export interface RuntimeChecks { /** * Verifies that action types are not registered more than once */ - strictActionTypeUniqueness: boolean; + strictActionTypeUniqueness?: boolean; } diff --git a/modules/store/src/runtime_checks.ts b/modules/store/src/runtime_checks.ts index 882900cfc5..6b01cae3dd 100644 --- a/modules/store/src/runtime_checks.ts +++ b/modules/store/src/runtime_checks.ts @@ -44,10 +44,10 @@ export function createSerializationCheckMetaReducer({ strictActionSerializability, strictStateSerializability, }: RuntimeChecks): MetaReducer { - return reducer => + return (reducer) => strictActionSerializability || strictStateSerializability ? serializationCheckMetaReducer(reducer, { - action: action => + action: (action) => strictActionSerializability && !ignoreNgrxAction(action), state: () => strictStateSerializability, }) @@ -58,10 +58,10 @@ export function createImmutabilityCheckMetaReducer({ strictActionImmutability, strictStateImmutability, }: RuntimeChecks): MetaReducer { - return reducer => + return (reducer) => strictActionImmutability || strictStateImmutability ? immutabilityCheckMetaReducer(reducer, { - action: action => + action: (action) => strictActionImmutability && !ignoreNgrxAction(action), state: () => strictStateImmutability, }) @@ -75,10 +75,10 @@ function ignoreNgrxAction(action: Action) { export function createInNgZoneCheckMetaReducer({ strictActionWithinNgZone, }: RuntimeChecks): MetaReducer { - return reducer => + return (reducer) => strictActionWithinNgZone ? inNgZoneAssertMetaReducer(reducer, { - action: action => + action: (action) => strictActionWithinNgZone && !ignoreNgrxAction(action), }) : reducer; @@ -140,19 +140,19 @@ export function _runtimeChecksFactory( return runtimeChecks; } -export function _actionTypeUniquenessCheck(config: RuntimeChecks) { +export function _actionTypeUniquenessCheck(config: RuntimeChecks): void { if (!config.strictActionTypeUniqueness) { return; } const duplicates = Object.entries(REGISTERED_ACTION_TYPES) - .filter(([_type, registrations]) => registrations > 1) + .filter(([, registrations]) => registrations > 1) .map(([type]) => type); if (duplicates.length) { throw new Error( `Action types are registered more than once, ${duplicates - .map(type => `"${type}"`) + .map((type) => `"${type}"`) .join(', ')}. ${RUNTIME_CHECK_URL}#strictactiontypeuniqueness` ); }