From 560872b0fa3e358597bc36a1404f787e8111ead8 Mon Sep 17 00:00:00 2001 From: Artur Androsovych Date: Tue, 27 Apr 2021 14:26:02 +0300 Subject: [PATCH] perf(store): tree-shake dev errors and warnings in prod (#1732) * perf(store): tree-shake errors and warnings * fix: do not emit `@internal` declarations * chore: update CHANGELOG.md --- CHANGELOG.md | 1 + bundlesize.config.json | 4 +- .../bundlesize.config.json | 2 +- packages/store/internals/src/angular.ts | 2 + packages/store/src/configs/messages.config.ts | 75 ++++++++++++------- packages/store/src/decorators/action.ts | 12 ++- .../src/decorators/select/select-factory.ts | 5 +- .../store/src/decorators/select/symbols.ts | 1 + .../store/src/decorators/selector/selector.ts | 12 ++- packages/store/src/decorators/state.ts | 15 +++- ...ch-outside-zone-ngxs-execution-strategy.ts | 28 ++++--- packages/store/src/internal/internals.ts | 11 ++- packages/store/src/internal/state-factory.ts | 21 ++++-- .../ivy/ensure-state-class-is-injectable.ts | 23 ------ .../store/src/ivy/ivy-enabled-in-dev-mode.ts | 54 ++++++++----- .../store/src/modules/ngxs-root.module.ts | 8 +- packages/store/src/symbols.ts | 5 ++ packages/store/src/utils/store-validators.ts | 54 ++++++------- packages/store/tests/action-handler.spec.ts | 3 +- ...tside-zone-ngxs-execution-strategy.spec.ts | 5 +- packages/store/tests/select.spec.ts | 7 +- packages/store/tests/selector.spec.ts | 5 +- packages/store/tests/state.spec.ts | 13 ++-- packages/store/tests/store-validator.spec.ts | 14 ++-- setupJest.ts | 2 + tsconfig.build.json | 1 + 26 files changed, 219 insertions(+), 164 deletions(-) delete mode 100644 packages/store/src/ivy/ensure-state-class-is-injectable.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 86e6d6aab..3f5298a47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ $ npm install @ngxs/store@dev ``` - Fix: Allow to inject the `Store` into the custom error handler [#1708](https://github.com/ngxs/store/pull/1708) +- Performance: Tree-shake errors and warnings [#1732](https://github.com/ngxs/store/pull/1732) - Performance: Storage Plugin - Tree-shake `console.*` calls and expand error messages [#1727](https://github.com/ngxs/store/pull/1727) - CI: Add bundlesize check for the latest integration app [#1710](https://github.com/ngxs/store/pull/1710) diff --git a/bundlesize.config.json b/bundlesize.config.json index dca673cdb..e5d458a01 100644 --- a/bundlesize.config.json +++ b/bundlesize.config.json @@ -32,14 +32,14 @@ "path": "./@ngxs/store/fesm2015/ngxs-store.js", "package": "@ngxs/store", "target": "es2015", - "maxSize": "114.14KB", + "maxSize": "116.30KB", "compression": "none" }, { "path": "./@ngxs/store/fesm5/ngxs-store.js", "package": "@ngxs/store", "target": "es5", - "maxSize": "134.18KB", + "maxSize": "135.86KB", "compression": "none" }, { diff --git a/integrations/hello-world-ng11-ivy/bundlesize.config.json b/integrations/hello-world-ng11-ivy/bundlesize.config.json index 85927fc56..ce93f7c29 100644 --- a/integrations/hello-world-ng11-ivy/bundlesize.config.json +++ b/integrations/hello-world-ng11-ivy/bundlesize.config.json @@ -3,7 +3,7 @@ { "path": "./dist-integration/main.*.js", "target": "es2015", - "maxSize": "253.54 kB", + "maxSize": "251.70 kB", "compression": "none" } ] diff --git a/packages/store/internals/src/angular.ts b/packages/store/internals/src/angular.ts index 32da67f80..e455daaa3 100644 --- a/packages/store/internals/src/angular.ts +++ b/packages/store/internals/src/angular.ts @@ -1,6 +1,8 @@ import { getPlatform, COMPILER_OPTIONS, CompilerOptions, PlatformRef } from '@angular/core'; import { memoize } from './memoize'; +// TODO: tree-shake this away in production since we're using it only to check whether +// NGXS is running in test mode! function _isAngularInTestMode() { const platformRef: PlatformRef | null = getPlatform(); if (!platformRef) return false; diff --git a/packages/store/src/configs/messages.config.ts b/packages/store/src/configs/messages.config.ts index bc6c75c48..0f416c674 100644 --- a/packages/store/src/configs/messages.config.ts +++ b/packages/store/src/configs/messages.config.ts @@ -1,26 +1,13 @@ export enum VALIDATION_CODE { - STATE_NAME = 'STATE_NAME', - STATE_UNIQUE = 'STATE_UNIQUE', - STATE_NAME_PROPERTY = 'STATE_NAME_PROPERTY', - STATE_DECORATOR = 'STATE_DECORATOR', INCORRECT_PRODUCTION = 'INCORRECT_PRODUCTION', INCORRECT_DEVELOPMENT = 'INCORRECT_DEVELOPMENT', SELECT_FACTORY_NOT_CONNECTED = 'SELECT_FACTORY_NOT_CONNECTED', - ACTION_DECORATOR = 'ACTION_DECORATOR', - SELECTOR_DECORATOR = 'SELECTOR_DECORATOR', - ZONE_WARNING = 'ZONE_WARNING', PATCHING_ARRAY = 'PATCHING_ARRAY', - PATCHING_PRIMITIVE = 'PATCHING_PRIMITIVE', - UNDECORATED_STATE_IN_IVY = 'UNDECORATED_STATE_IN_IVY' + PATCHING_PRIMITIVE = 'PATCHING_PRIMITIVE' } +// TODO: these messages might be tree-shaken away in the future. export const CONFIG_MESSAGES = { - [VALIDATION_CODE.STATE_NAME]: (name: string) => - `${name} is not a valid state name. It needs to be a valid object property name.`, - [VALIDATION_CODE.STATE_NAME_PROPERTY]: () => `States must register a 'name' property`, - [VALIDATION_CODE.STATE_UNIQUE]: (current: string, newName: string, oldName: string) => - `State name '${current}' from ${newName} already exists in ${oldName}`, - [VALIDATION_CODE.STATE_DECORATOR]: () => 'States must be decorated with @State() decorator', [VALIDATION_CODE.INCORRECT_PRODUCTION]: () => 'Angular is running in production mode but NGXS is still running in the development mode!\n' + 'Please set developmentMode to false on the NgxsModule options when in production mode.\n' + @@ -30,15 +17,53 @@ export const CONFIG_MESSAGES = { 'NgxsModule.forRoot(states, { developmentMode: !environment.production })', [VALIDATION_CODE.SELECT_FACTORY_NOT_CONNECTED]: () => 'You have forgotten to import the NGXS module!', - [VALIDATION_CODE.ACTION_DECORATOR]: () => - '@Action() decorator cannot be used with static methods', - [VALIDATION_CODE.SELECTOR_DECORATOR]: () => 'Selectors only work on methods', - [VALIDATION_CODE.ZONE_WARNING]: () => - 'Your application was bootstrapped with nooped zone and your execution strategy requires an actual NgZone!\n' + - 'Please set the value of the executionStrategy property to NoopNgxsExecutionStrategy.\n' + - 'NgxsModule.forRoot(states, { executionStrategy: NoopNgxsExecutionStrategy })', + // This can be a breaking change if we don't throw these errors even in production mode. [VALIDATION_CODE.PATCHING_ARRAY]: () => 'Patching arrays is not supported.', - [VALIDATION_CODE.PATCHING_PRIMITIVE]: () => 'Patching primitives is not supported.', - [VALIDATION_CODE.UNDECORATED_STATE_IN_IVY]: (name: string) => - `'${name}' class should be decorated with @Injectable() right after the @State() decorator` + [VALIDATION_CODE.PATCHING_PRIMITIVE]: () => 'Patching primitives is not supported.' }; + +// The below functions are decoupled from the `CONFIG_MESSAGES` object for now, since object properties +// are not tree-shakable. That means that if the error is thrown only in development mode it still will get +// bundled into the final file. This is how Angular does error tree-shaking internally. + +export function throwStateNameError(name: string): never { + throw new Error( + `${name} is not a valid state name. It needs to be a valid object property name.` + ); +} + +export function throwStateNamePropertyError(): never { + throw new Error(`States must register a 'name' property.`); +} + +export function throwStateUniqueError( + current: string, + newName: string, + oldName: string +): never { + throw new Error(`State name '${current}' from ${newName} already exists in ${oldName}.`); +} + +export function throwStateDecoratorError(name: string): never { + throw new Error(`States must be decorated with @State() decorator, but "${name}" isn't.`); +} + +export function throwActionDecoratorError(): never { + throw new Error('@Action() decorator cannot be used with static methods.'); +} + +export function throwSelectorDecoratorError(): never { + throw new Error('Selectors only work on methods.'); +} + +export function getZoneWarningMessage(): string { + return ( + 'Your application was bootstrapped with nooped zone and your execution strategy requires an actual NgZone!\n' + + 'Please set the value of the executionStrategy property to NoopNgxsExecutionStrategy.\n' + + 'NgxsModule.forRoot(states, { executionStrategy: NoopNgxsExecutionStrategy })' + ); +} + +export function getUndecoratedStateInIvyWarningMessage(name: string): string { + return `'${name}' class should be decorated with @Injectable() right after the @State() decorator`; +} diff --git a/packages/store/src/decorators/action.ts b/packages/store/src/decorators/action.ts index c990b26f8..a3e54b6bf 100644 --- a/packages/store/src/decorators/action.ts +++ b/packages/store/src/decorators/action.ts @@ -1,6 +1,6 @@ import { ensureStoreMetadata } from '../internal/internals'; import { ActionType, ActionOptions } from '../actions/symbols'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../configs/messages.config'; +import { throwActionDecoratorError } from '../configs/messages.config'; /** * Decorates a method with a action information. @@ -10,10 +10,14 @@ export function Action( options?: ActionOptions ): MethodDecorator { return (target: any, name: string | symbol): void => { - const isStaticMethod = target.hasOwnProperty('prototype'); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + const isStaticMethod = target.hasOwnProperty('prototype'); - if (isStaticMethod) { - throw new Error(CONFIG_MESSAGES[VALIDATION_CODE.ACTION_DECORATOR]()); + if (isStaticMethod) { + throwActionDecoratorError(); + } } const meta = ensureStoreMetadata(target.constructor); diff --git a/packages/store/src/decorators/select/select-factory.ts b/packages/store/src/decorators/select/select-factory.ts index 90d5291c5..87304fba9 100644 --- a/packages/store/src/decorators/select/select-factory.ts +++ b/packages/store/src/decorators/select/select-factory.ts @@ -4,9 +4,8 @@ import { Store } from '../../store'; import { NgxsConfig } from '../../symbols'; /** - * Allows the select decorator to get access to the DI store. - * @internal only use in @Select decorator - * @ignore + * Allows the select decorator to get access to the DI store, this is used internally + * in `@Select` decorator. */ @Injectable() export class SelectFactory implements OnDestroy { diff --git a/packages/store/src/decorators/select/symbols.ts b/packages/store/src/decorators/select/symbols.ts index 2b24171f7..01d73be32 100644 --- a/packages/store/src/decorators/select/symbols.ts +++ b/packages/store/src/decorators/select/symbols.ts @@ -9,6 +9,7 @@ import { ExtractTokenType } from '../../state-token/symbols'; const DOLLAR_CHAR_CODE = 36; export function createSelectObservable(selector: any): Observable { + // We're not adding `ngDevMode` guard since we have still to throw this error until we fix SSR issue. if (!SelectFactory.store) { throw new Error(CONFIG_MESSAGES[VALIDATION_CODE.SELECT_FACTORY_NOT_CONNECTED]()); } diff --git a/packages/store/src/decorators/selector/selector.ts b/packages/store/src/decorators/selector/selector.ts index 463713208..e101178e7 100644 --- a/packages/store/src/decorators/selector/selector.ts +++ b/packages/store/src/decorators/selector/selector.ts @@ -1,4 +1,4 @@ -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../../configs/messages.config'; +import { throwSelectorDecoratorError } from '../../configs/messages.config'; import { createSelector } from '../../utils/selector-utils'; import { SelectorSpec, SelectorType } from './symbols'; @@ -11,10 +11,14 @@ export function Selector(selectors?: T[]): SelectorType { key: string | symbol, descriptor: TypedPropertyDescriptor> ): TypedPropertyDescriptor> | void => { - const isNotMethod = !(descriptor && descriptor.value !== null); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + const isNotMethod = !(descriptor && descriptor.value !== null); - if (isNotMethod) { - throw new Error(CONFIG_MESSAGES[VALIDATION_CODE.SELECTOR_DECORATOR]()); + if (isNotMethod) { + throwSelectorDecoratorError(); + } } const originalFn = descriptor.value; diff --git a/packages/store/src/decorators/state.ts b/packages/store/src/decorators/state.ts index 36dd94bc3..958b88432 100644 --- a/packages/store/src/decorators/state.ts +++ b/packages/store/src/decorators/state.ts @@ -3,7 +3,7 @@ import { StateClass } from '@ngxs/store/internals'; import { ensureStoreMetadata, MetaDataModel, StateClassInternal } from '../internal/internals'; import { META_KEY, META_OPTIONS_KEY, StoreOptions } from '../symbols'; import { StoreValidators } from '../utils/store-validators'; -import { ensureStateClassIsInjectable } from '../ivy/ensure-state-class-is-injectable'; +import { ensureStateClassIsInjectable } from '../ivy/ivy-enabled-in-dev-mode'; interface MutateMetaOptions { meta: MetaDataModel; @@ -26,7 +26,12 @@ export function State(options: StoreOptions) { const { children, defaults, name } = optionsWithInheritance; const stateName: string | null = typeof name === 'string' ? name : (name && name.getName()) || null; - StoreValidators.checkCorrectStateName(stateName); + + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + StoreValidators.checkThatStateIsNamedCorrectly(stateName); + } if (inheritedStateClass.hasOwnProperty(META_KEY)) { const inheritedMeta: Partial = inheritedStateClass[META_KEY] || {}; @@ -39,7 +44,11 @@ export function State(options: StoreOptions) { } return (target: StateClass): void => { - ensureStateClassIsInjectable(target); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + ensureStateClassIsInjectable(target); + } const stateClass: StateClassInternal = target; const meta: MetaDataModel = ensureStoreMetadata(stateClass); const inheritedStateClass: StateClassInternal = Object.getPrototypeOf(stateClass); diff --git a/packages/store/src/execution/dispatch-outside-zone-ngxs-execution-strategy.ts b/packages/store/src/execution/dispatch-outside-zone-ngxs-execution-strategy.ts index 00869eafb..daf5b10aa 100644 --- a/packages/store/src/execution/dispatch-outside-zone-ngxs-execution-strategy.ts +++ b/packages/store/src/execution/dispatch-outside-zone-ngxs-execution-strategy.ts @@ -2,12 +2,16 @@ import { Inject, Injectable, NgZone, PLATFORM_ID } from '@angular/core'; import { isPlatformServer } from '@angular/common'; import { NgxsExecutionStrategy } from './symbols'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../configs/messages.config'; +import { getZoneWarningMessage } from '../configs/messages.config'; @Injectable() export class DispatchOutsideZoneNgxsExecutionStrategy implements NgxsExecutionStrategy { constructor(private _ngZone: NgZone, @Inject(PLATFORM_ID) private _platformId: string) { - this.verifyZoneIsNotNooped(this._ngZone); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + verifyZoneIsNotNooped(_ngZone); + } } enter(func: () => T): T { @@ -34,15 +38,17 @@ export class DispatchOutsideZoneNgxsExecutionStrategy implements NgxsExecutionSt } return func(); } +} - private verifyZoneIsNotNooped(ngZone: NgZone): void { - // `NoopNgZone` is not exposed publicly as it doesn't expect - // to be used outside of the core Angular code, thus we just have - // to check if the zone doesn't extend or instanceof `NgZone` - if (ngZone instanceof NgZone) { - return; - } - - console.warn(CONFIG_MESSAGES[VALIDATION_CODE.ZONE_WARNING]()); +// Caretaker note: this should exist as a separate function and not a class method, +// since class methods are not tree-shakable. +function verifyZoneIsNotNooped(ngZone: NgZone): void { + // `NoopNgZone` is not exposed publicly as it doesn't expect + // to be used outside of the core Angular code, thus we just have + // to check if the zone doesn't extend or instanceof `NgZone`. + if (ngZone instanceof NgZone) { + return; } + + console.warn(getZoneWarningMessage()); } diff --git a/packages/store/src/internal/internals.ts b/packages/store/src/internal/internals.ts index ecf0f001c..52822d9fb 100644 --- a/packages/store/src/internal/internals.ts +++ b/packages/store/src/internal/internals.ts @@ -216,13 +216,16 @@ export function propGetter(paths: string[], config: NgxsConfig) { export function buildGraph(stateClasses: StateClassInternal[]): StateKeyGraph { const findName = (stateClass: StateClassInternal) => { const meta = stateClasses.find(g => g === stateClass); - if (!meta) { + + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if ((typeof ngDevMode === 'undefined' || ngDevMode) && !meta) { throw new Error( `Child state not found: ${stateClass}. \r\nYou may have forgotten to add states to module` ); } - return meta[META_KEY]!.name!; + return meta![META_KEY]!.name!; }; return stateClasses.reduce( @@ -332,7 +335,9 @@ export function topologicalSort(graph: StateKeyGraph): string[] { visited[name] = true; graph[name].forEach((dep: string) => { - if (ancestors.indexOf(dep) >= 0) { + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if ((typeof ngDevMode === 'undefined' || ngDevMode) && ancestors.indexOf(dep) >= 0) { throw new Error( `Circular dependency '${dep}' is required by '${name}': ${ancestors.join(' -> ')}` ); diff --git a/packages/store/src/internal/state-factory.ts b/packages/store/src/internal/state-factory.ts index 5608f52b2..3cacaec48 100644 --- a/packages/store/src/internal/state-factory.ts +++ b/packages/store/src/internal/state-factory.ts @@ -25,7 +25,8 @@ import { StatesByName, topologicalSort, RuntimeSelectorContext, - SharedSelectorOptions + SharedSelectorOptions, + getStoreMetadata } from './internals'; import { getActionTypeFromInstance, getValue, setValue } from '../utils/utils'; import { ofActionDispatched } from '../operators/of-action'; @@ -126,10 +127,6 @@ export class StateFactory implements OnDestroy { return value; } - private static checkStatesAreValid(stateClasses: StateClassInternal[]): void { - stateClasses.forEach(StoreValidators.getValidStateMeta); - } - ngOnDestroy(): void { // I'm using non-null assertion here since `_actionsSubscrition` will // be 100% defined. This is because `ngOnDestroy()` cannot be invoked @@ -142,7 +139,12 @@ export class StateFactory implements OnDestroy { * Add a new state to the global defs. */ add(stateClasses: StateClassInternal[]): MappedStore[] { - StateFactory.checkStatesAreValid(stateClasses); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + StoreValidators.checkThatStateClassesHaveBeenDecorated(stateClasses); + } + const { newStates } = this.addToStatesMap(stateClasses); if (!newStates.length) return []; @@ -280,7 +282,12 @@ export class StateFactory implements OnDestroy { const statesMap: StatesByName = this.statesByName; for (const stateClass of stateClasses) { - const stateName: string = StoreValidators.checkStateNameIsUnique(stateClass, statesMap); + const stateName = getStoreMetadata(stateClass).name!; + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + StoreValidators.checkThatStateNameIsUnique(stateName, stateClass, statesMap); + } const unmountedState = !statesMap[stateName]; if (unmountedState) { newStates.push(stateClass); diff --git a/packages/store/src/ivy/ensure-state-class-is-injectable.ts b/packages/store/src/ivy/ensure-state-class-is-injectable.ts deleted file mode 100644 index 32687c4b7..000000000 --- a/packages/store/src/ivy/ensure-state-class-is-injectable.ts +++ /dev/null @@ -1,23 +0,0 @@ -import { ivyEnabledInDevMode$ } from './ivy-enabled-in-dev-mode'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../configs/messages.config'; - -/** - * All provided or injected tokens must have `@Injectable` decorator - * (previously, injected tokens without `@Injectable` were allowed - * if another decorator was used, e.g. pipes). - */ -export function ensureStateClassIsInjectable(target: any): void { - // `ɵprov` is a static property added by the NGCC compiler. It always exists in - // AOT mode because this property is added before runtime. If an application is running in - // JIT mode then this property can be added by the `@Injectable()` decorator. The `@Injectable()` - // decorator has to go after the `@State()` decorator, thus we prevent users from unwanted DI errors. - ivyEnabledInDevMode$.subscribe(_ivyEnabledInDevMode => { - if (_ivyEnabledInDevMode) { - const ngInjectableDef = target.ɵprov; - if (!ngInjectableDef) { - // Don't warn if Ivy is disabled or `ɵprov` exists on the class - console.warn(CONFIG_MESSAGES[VALIDATION_CODE.UNDECORATED_STATE_IN_IVY](target.name)); - } - } - }); -} diff --git a/packages/store/src/ivy/ivy-enabled-in-dev-mode.ts b/packages/store/src/ivy/ivy-enabled-in-dev-mode.ts index 26717476d..e78901d6a 100644 --- a/packages/store/src/ivy/ivy-enabled-in-dev-mode.ts +++ b/packages/store/src/ivy/ivy-enabled-in-dev-mode.ts @@ -1,30 +1,48 @@ -import { isDevMode } from '@angular/core'; import { ReplaySubject } from 'rxjs'; -export const ivyEnabledInDevMode$ = new ReplaySubject(1); +import { getUndecoratedStateInIvyWarningMessage } from '../configs/messages.config'; + +// We've got such expression because Terser doesn't tree-shake `new SomeClass()` expressions +// since Terser considers them as "side-effectful". E.g. `new InjectionToken()` expression isn't +// tree-shakable even if the token has `providedIn: root` and is not used. +// The `ReplaySubject` will be subscribed inside the `ngDevMode` guard anyway. +export const ivyEnabledInDevMode$ = + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + typeof ngDevMode === 'undefined' || ngDevMode ? new ReplaySubject(1) : null!; -/** - * Ivy exposes helper functions to the global `window.ng` object. - * Those functions are `getComponent, getContext, - * getListeners, getViewComponent, getHostElement, getInjector, - * getRootComponents, getDirectives, getDebugNode` - * Previously, old view engine exposed `window.ng.coreTokens` and - * `window.ng.probe` if an application was in development/production. - * Ivy doesn't expose these functions in production. Developers will be able - * to see warnings in both JIT/AOT modes, but only if an application - * is in development. - */ export function setIvyEnabledInDevMode(): void { try { - // `try-catch` will also handle server-side rendering, as - // `window is not defined` will not be thrown. const ng = (window as any).ng; - const _viewEngineEnabled = !!ng.probe && !!ng.coreTokens; - const _ivyEnabledInDevMode = !_viewEngineEnabled && isDevMode(); - ivyEnabledInDevMode$.next(_ivyEnabledInDevMode); + const viewEngineEnabled = !!ng.probe && !!ng.coreTokens; + const ivyEnabledInDevMode = + !viewEngineEnabled && typeof ngDevMode !== 'undefined' && !!ngDevMode; + // The `ngDevMode` is NOT set for projects that are still using the View Engine template compiler. + ivyEnabledInDevMode$.next(ivyEnabledInDevMode); } catch { ivyEnabledInDevMode$.next(false); } finally { ivyEnabledInDevMode$.complete(); } } + +/** + * All provided or injected tokens must have `@Injectable` decorator + * (previously, injected tokens without `@Injectable` were allowed + * if another decorator was used, e.g. pipes). + */ +export function ensureStateClassIsInjectable(target: any): void { + // `ɵprov` is a static property added by the NGCC compiler. It always exists in + // AOT mode because this property is added before runtime. If an application is running in + // JIT mode then this property can be added by the `@Injectable()` decorator. The `@Injectable()` + // decorator has to go after the `@State()` decorator, thus we prevent users from unwanted DI errors. + ivyEnabledInDevMode$.subscribe(_ivyEnabledInDevMode => { + if (_ivyEnabledInDevMode) { + const ngInjectableDef = target.ɵprov; + if (!ngInjectableDef) { + // Don't warn if Ivy is disabled or `ɵprov` exists on the class + console.warn(getUndecoratedStateInIvyWarningMessage(target.name)); + } + } + }); +} diff --git a/packages/store/src/modules/ngxs-root.module.ts b/packages/store/src/modules/ngxs-root.module.ts index 27de397d2..d42da4940 100644 --- a/packages/store/src/modules/ngxs-root.module.ts +++ b/packages/store/src/modules/ngxs-root.module.ts @@ -26,8 +26,12 @@ export class NgxsRootModule { states: StateClassInternal[] = [], lifecycleStateManager: LifecycleStateManager ) { - // Validate states on having the `@Injectable()` decorator in Ivy - setIvyEnabledInDevMode(); + // Caretaker note: we have still left the `typeof` condition in order to avoid + // creating a breaking change for projects that still use the View Engine. + if (typeof ngDevMode === 'undefined' || ngDevMode) { + // Validate states on having the `@Injectable()` decorator in Ivy + setIvyEnabledInDevMode(); + } // Add stores to the state graph and return their defaults const results: StatesAndDefaults = factory.addAndReturnDefaults(states); diff --git a/packages/store/src/symbols.ts b/packages/store/src/symbols.ts index 63c006704..872bc1ce2 100644 --- a/packages/store/src/symbols.ts +++ b/packages/store/src/symbols.ts @@ -170,3 +170,8 @@ export interface NgxsAfterBootstrap { } export type NgxsModuleOptions = Partial; + +/** @internal */ +declare global { + const ngDevMode: boolean; +} diff --git a/packages/store/src/utils/store-validators.ts b/packages/store/src/utils/store-validators.ts index acb37c148..2289ea8fe 100644 --- a/packages/store/src/utils/store-validators.ts +++ b/packages/store/src/utils/store-validators.ts @@ -1,50 +1,40 @@ +import { getStoreMetadata, StateClassInternal, StatesByName } from '../internal/internals'; import { - getStoreMetadata, - MetaDataModel, - StateClassInternal, - StatesByName -} from '../internal/internals'; -import { - CONFIG_MESSAGES as MESSAGES, - VALIDATION_CODE as CODE + throwStateDecoratorError, + throwStateNameError, + throwStateNamePropertyError, + throwStateUniqueError } from '../configs/messages.config'; export abstract class StoreValidators { - public static stateNameRegex: RegExp = new RegExp('^[a-zA-Z0-9_]+$'); - - public static stateNameErrorMessage(name: string) { - return MESSAGES[CODE.STATE_NAME](name); - } + private static stateNameRegex: RegExp = new RegExp('^[a-zA-Z0-9_]+$'); - public static checkCorrectStateName(name: string | null) { + static checkThatStateIsNamedCorrectly(name: string | null): void | never { if (!name) { - throw new Error(MESSAGES[CODE.STATE_NAME_PROPERTY]()); - } - - if (!this.stateNameRegex.test(name)) { - throw new Error(this.stateNameErrorMessage(name)); + throwStateNamePropertyError(); + } else if (!this.stateNameRegex.test(name)) { + throwStateNameError(name); } } - public static checkStateNameIsUnique( + static checkThatStateNameIsUnique( + stateName: string, state: StateClassInternal, statesByName: StatesByName - ): string { - const meta: MetaDataModel = this.getValidStateMeta(state); - const stateName: string = meta!.name as string; + ): void | never { const existingState = statesByName[stateName]; if (existingState && existingState !== state) { - throw new Error(MESSAGES[CODE.STATE_UNIQUE](stateName, state.name, existingState.name)); + throwStateUniqueError(stateName, state.name, existingState.name); } - return stateName; } - public static getValidStateMeta(state: StateClassInternal): MetaDataModel { - const meta: MetaDataModel = getStoreMetadata(state); - if (!meta) { - throw new Error(MESSAGES[CODE.STATE_DECORATOR]()); - } - - return meta; + static checkThatStateClassesHaveBeenDecorated( + stateClasses: StateClassInternal[] + ): void | never { + stateClasses.forEach((stateClass: StateClassInternal) => { + if (!getStoreMetadata(stateClass)) { + throwStateDecoratorError(stateClass.name); + } + }); } } diff --git a/packages/store/tests/action-handler.spec.ts b/packages/store/tests/action-handler.spec.ts index fa4fa0791..2ea063181 100644 --- a/packages/store/tests/action-handler.spec.ts +++ b/packages/store/tests/action-handler.spec.ts @@ -11,7 +11,6 @@ import { NgxsModule } from '../src/module'; import { Store } from '../src/store'; import { Actions } from '../src/actions-stream'; import { NoopErrorHandler } from './helpers/utils'; -import { VALIDATION_CODE, CONFIG_MESSAGES } from '../src/configs/messages.config'; describe('Action handlers', () => { class TestAction { @@ -51,7 +50,7 @@ describe('Action handlers', () => { new CounterState(); } catch ({ message }) { - expect(message).toEqual(CONFIG_MESSAGES[VALIDATION_CODE.ACTION_DECORATOR]()); + expect(message).toEqual('@Action() decorator cannot be used with static methods.'); } }); diff --git a/packages/store/tests/execution/dispatch-outside-zone-ngxs-execution-strategy.spec.ts b/packages/store/tests/execution/dispatch-outside-zone-ngxs-execution-strategy.spec.ts index 1c9c0ebfd..ceff7a9c8 100644 --- a/packages/store/tests/execution/dispatch-outside-zone-ngxs-execution-strategy.spec.ts +++ b/packages/store/tests/execution/dispatch-outside-zone-ngxs-execution-strategy.spec.ts @@ -15,7 +15,7 @@ import { Select, Actions } from '../../src/public_api'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../..//src/configs/messages.config'; +import { getZoneWarningMessage } from '../../src/configs/messages.config'; import { DispatchOutsideZoneNgxsExecutionStrategy } from '../../src/execution/dispatch-outside-zone-ngxs-execution-strategy'; describe('DispatchOutsideZoneNgxsExecutionStrategy', () => { @@ -320,8 +320,7 @@ describe('DispatchOutsideZoneNgxsExecutionStrategy', () => { try { // Assert - const ZONE_WARNING = CONFIG_MESSAGES[VALIDATION_CODE.ZONE_WARNING](); - expect(spy).toHaveBeenCalledWith(ZONE_WARNING); + expect(spy).toHaveBeenCalledWith(getZoneWarningMessage()); } finally { spy.mockRestore(); } diff --git a/packages/store/tests/select.spec.ts b/packages/store/tests/select.spec.ts index fd3e61cc0..9784ae650 100644 --- a/packages/store/tests/select.spec.ts +++ b/packages/store/tests/select.spec.ts @@ -11,7 +11,6 @@ import { Selector } from '../src/decorators/selector/selector'; import { Select } from '../src/decorators/select/select'; import { StateContext } from '../src/symbols'; import { removeDollarAtTheEnd } from '../src/decorators/select/symbols'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../src/configs/messages.config'; describe('Select', () => { interface SubSubStateModel { @@ -79,10 +78,8 @@ describe('Select', () => { } new SelectComponent().state.subscribe(); - } catch (e) { - expect(e.message).toEqual( - CONFIG_MESSAGES[VALIDATION_CODE.SELECT_FACTORY_NOT_CONNECTED]() - ); + } catch ({ message }) { + expect(message).toEqual('You have forgotten to import the NGXS module!'); } }); diff --git a/packages/store/tests/selector.spec.ts b/packages/store/tests/selector.spec.ts index 3d1aca137..b0a5b510f 100644 --- a/packages/store/tests/selector.spec.ts +++ b/packages/store/tests/selector.spec.ts @@ -8,7 +8,6 @@ import { NgxsModule } from '../src/module'; import { Selector } from '../src/decorators/selector/selector'; import { NgxsConfig } from '../src/symbols'; import { SelectorOptions } from '../src/decorators/selector-options'; -import { CONFIG_MESSAGES, VALIDATION_CODE } from '../src/configs/messages.config'; import { Injectable } from '@angular/core'; describe('Selector', () => { @@ -843,8 +842,8 @@ describe('Selector', () => { } new MyComponent(); - } catch (e) { - expect(e.message).toEqual(CONFIG_MESSAGES[VALIDATION_CODE.SELECTOR_DECORATOR]()); + } catch ({ message }) { + expect(message).toEqual('Selectors only work on methods.'); } }); }); diff --git a/packages/store/tests/state.spec.ts b/packages/store/tests/state.spec.ts index 40b2860e8..30ede723c 100644 --- a/packages/store/tests/state.spec.ts +++ b/packages/store/tests/state.spec.ts @@ -16,12 +16,7 @@ import { import { InitState, UpdateState } from '../src/actions/actions'; import { Action, NgxsModule, NgxsOnInit, State, StateContext, Store } from '../src/public_api'; import { META_KEY, NgxsAfterBootstrap } from '../src/symbols'; -import { StoreValidators } from '../src/utils/store-validators'; import { simplePatch } from '../src/internal/state-operators'; -import { - CONFIG_MESSAGES as MESSAGES, - VALIDATION_CODE as CODE -} from '../src/configs/messages.config'; describe('State', () => { it('describes correct name', () => { @@ -83,7 +78,9 @@ describe('State', () => { message = err.message; } - expect(message).toBe(StoreValidators.stateNameErrorMessage('bar-foo')); + expect(message).toEqual( + 'bar-foo is not a valid state name. It needs to be a valid object property name.' + ); }); it('should throw when state parameters are not passed', () => { @@ -95,8 +92,8 @@ describe('State', () => { TestBed.configureTestingModule({ imports: [NgxsModule.forRoot([MyOtherState])] }); - } catch (err) { - expect(err.message).toEqual(MESSAGES[CODE.STATE_NAME_PROPERTY]()); + } catch ({ message }) { + expect(message).toEqual(`States must register a 'name' property.`); } }); diff --git a/packages/store/tests/store-validator.spec.ts b/packages/store/tests/store-validator.spec.ts index e07bac2b8..0a8b45d10 100644 --- a/packages/store/tests/store-validator.spec.ts +++ b/packages/store/tests/store-validator.spec.ts @@ -32,7 +32,7 @@ describe('StoreValidator', () => { errorMessage = e.message; } expect(errorMessage).toEqual( - `State name 'duplicate' from MyDuplicateState already exists in MyOtherState` + `State name 'duplicate' from MyDuplicateState already exists in MyOtherState.` ); }); @@ -67,7 +67,7 @@ describe('StoreValidator', () => { errorMessage = e.message; } expect(errorMessage).toEqual( - `State name 'duplicate' from MyDuplicateChildState already exists in MyOtherState` + `State name 'duplicate' from MyDuplicateChildState already exists in MyOtherState.` ); }); @@ -98,7 +98,7 @@ describe('StoreValidator', () => { errorMessage = e.message; } expect(errorMessage).toEqual( - `State name 'duplicate' from MyDuplicateState already exists in MyOtherState` + `State name 'duplicate' from MyDuplicateState already exists in MyOtherState.` ); }); @@ -148,7 +148,9 @@ describe('StoreValidator', () => { } catch (e) { errorMessage = e.message; } - expect(errorMessage).toEqual('States must be decorated with @State() decorator'); + expect(errorMessage).toEqual( + `States must be decorated with @State() decorator, but "TestState" isn't.` + ); }); it('should detect a missing decorator in child states', () => { @@ -171,7 +173,9 @@ describe('StoreValidator', () => { } catch (e) { errorMessage = e.message; } - expect(errorMessage).toEqual('States must be decorated with @State() decorator'); + expect(errorMessage).toEqual( + `States must be decorated with @State() decorator, but "ChildState" isn't.` + ); }); }); }); diff --git a/setupJest.ts b/setupJest.ts index 9f53b051d..4bf79e75a 100644 --- a/setupJest.ts +++ b/setupJest.ts @@ -47,3 +47,5 @@ if (CI) { jest.spyOn(global.console, methodName as any).mockImplementation(() => jest.fn()); }); } + +((global as unknown) as { ngDevMode: boolean }).ngDevMode = true; diff --git a/tsconfig.build.json b/tsconfig.build.json index 3b052d8d5..401a577e6 100644 --- a/tsconfig.build.json +++ b/tsconfig.build.json @@ -8,6 +8,7 @@ "inlineSources": true, "importHelpers": true, "skipLibCheck": true, + "stripInternal": true, "paths": { "@ngxs/*": ["@ngxs/*"] }