Skip to content

Commit

Permalink
perf(store): tree-shake dev errors and warnings in prod (ngxs#1732)
Browse files Browse the repository at this point in the history
* perf(store): tree-shake errors and warnings

* fix: do not emit `@internal` declarations

* chore: update CHANGELOG.md
  • Loading branch information
arturovt authored Apr 27, 2021
1 parent 2b6b818 commit 560872b
Show file tree
Hide file tree
Showing 26 changed files with 219 additions and 164 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ $ npm install @ngxs/store@dev
```

- 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)

Expand Down
4 changes: 2 additions & 2 deletions bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -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"
},
{
Expand Down
2 changes: 1 addition & 1 deletion integrations/hello-world-ng11-ivy/bundlesize.config.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
{
"path": "./dist-integration/main.*.js",
"target": "es2015",
"maxSize": "253.54 kB",
"maxSize": "251.70 kB",
"compression": "none"
}
]
Expand Down
2 changes: 2 additions & 0 deletions packages/store/internals/src/angular.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down
75 changes: 50 additions & 25 deletions packages/store/src/configs/messages.config.ts
Original file line number Diff line number Diff line change
@@ -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' +
Expand All @@ -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`;
}
12 changes: 8 additions & 4 deletions packages/store/src/decorators/action.ts
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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);
Expand Down
5 changes: 2 additions & 3 deletions packages/store/src/decorators/select/select-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions packages/store/src/decorators/select/symbols.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { ExtractTokenType } from '../../state-token/symbols';
const DOLLAR_CHAR_CODE = 36;

export function createSelectObservable<T = any>(selector: any): Observable<T> {
// 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]());
}
Expand Down
12 changes: 8 additions & 4 deletions packages/store/src/decorators/selector/selector.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -11,10 +11,14 @@ export function Selector<T>(selectors?: T[]): SelectorType<T> {
key: string | symbol,
descriptor: TypedPropertyDescriptor<SelectorSpec<T, U>>
): TypedPropertyDescriptor<SelectorSpec<T, U>> | 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;
Expand Down
15 changes: 12 additions & 3 deletions packages/store/src/decorators/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T> {
meta: MetaDataModel;
Expand All @@ -26,7 +26,12 @@ export function State<T>(options: StoreOptions<T>) {
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<MetaDataModel> = inheritedStateClass[META_KEY] || {};
Expand All @@ -39,7 +44,11 @@ export function State<T>(options: StoreOptions<T>) {
}

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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<T>(func: () => T): T {
Expand All @@ -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());
}
11 changes: 8 additions & 3 deletions packages/store/src/internal/internals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<StateKeyGraph>(
Expand Down Expand Up @@ -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(' -> ')}`
);
Expand Down
21 changes: 14 additions & 7 deletions packages/store/src/internal/state-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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
Expand All @@ -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 [];

Expand Down Expand Up @@ -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);
Expand Down
23 changes: 0 additions & 23 deletions packages/store/src/ivy/ensure-state-class-is-injectable.ts

This file was deleted.

Loading

0 comments on commit 560872b

Please sign in to comment.