Skip to content

Commit

Permalink
fix(store): select,selectOnce,selectSnapshot should only accept typ…
Browse files Browse the repository at this point in the history
…ed selector

This commit modifies the signatures of `select`, `selectOnce`, and `selectSnapshot` to
exclusively accept typed selectors. They should not permit acceptance of anything lacking
type information, such as state classes (`select(MyState)`), strings, or anonymous functions.
Only state tokens and selectors are now allowed.

This adjustment also impacts the router plugin, as it previously used `RouterState` for selecting
snapshots, which is no longer allowed. Consequently, we introduced `ROUTER_STATE_TOKEN` as the
replacement for selections involving `RouterState`.
  • Loading branch information
arturovt committed Mar 16, 2024
1 parent 296071a commit fc8d954
Show file tree
Hide file tree
Showing 12 changed files with 51 additions and 56 deletions.
2 changes: 1 addition & 1 deletion integration/app/app.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class AppComponent implements OnInit {

ngOnInit(): void {
const payload: Todo = 'ngOnInit todo';
const state: Todo[] = this._store.selectSnapshot(TodoState);
const state: Todo[] = this._store.selectSnapshot(TodoState.getTodoState);
if (!state.includes(payload)) {
this._store.dispatch(new AddTodo(payload));
}
Expand Down
2 changes: 1 addition & 1 deletion integration/app/list/list.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export class ListComponent {
hello$ = this._store.select(ListState.getHello);
hello = this._store.selectSignal(ListState.getHello);

router$ = this._store.select(RouterState.state);
router$ = this._store.select<RouterStateSnapshot | undefined>(RouterState.state);
router = this._store.selectSignal<RouterStateSnapshot | undefined>(RouterState.state);

constructor(private _store: Store) {}
Expand Down
4 changes: 2 additions & 2 deletions integrations/hello-world-ng16/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { Component } from '@angular/core';
import { Store } from '@ngxs/store';
import { CounterState, Increment } from './store';
import { COUNTER_STATE_TOKEN, Increment } from './store';

@Component({
selector: 'app-root',
templateUrl: './app.component.html'
})
export class AppComponent {
counter$ = this.store.select<number>(CounterState);
counter$ = this.store.select<number>(COUNTER_STATE_TOKEN);

constructor(private store: Store) {}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { Injectable } from '@angular/core';
import { State, Action, StateContext } from '@ngxs/store';
import { State, Action, StateContext, StateToken } from '@ngxs/store';

import { Increment } from './counter.actions';

export const COUNTER_STATE_TOKEN = new StateToken<number>('counter');

@State<number>({
name: 'counter',
name: COUNTER_STATE_TOKEN,
defaults: 0
})
@Injectable()
Expand Down
4 changes: 2 additions & 2 deletions integrations/hello-world-ng17/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { Component } from '@angular/core';
import { Store } from '@ngxs/store';
import { CounterState, Increment } from './store';
import { COUNTER_STATE_TOKEN, Increment } from './store';

@Component({
selector: 'app-root',
templateUrl: './app.component.html'
})
export class AppComponent {
counter$ = this.store.select<number>(CounterState);
counter$ = this.store.select<number>(COUNTER_STATE_TOKEN);

constructor(private store: Store) {}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import { Injectable } from '@angular/core';
import { State, Action, StateContext } from '@ngxs/store';
import { State, Action, StateContext, StateToken } from '@ngxs/store';

import { Increment } from './counter.actions';

export const COUNTER_STATE_TOKEN = new StateToken<number>('counter');

@State<number>({
name: 'counter',
name: COUNTER_STATE_TOKEN,
defaults: 0
})
@Injectable()
Expand Down
2 changes: 1 addition & 1 deletion packages/router-plugin/src/public_api.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
export { NgxsRouterPluginModule, withNgxsRouterPlugin } from './router.module';
export { RouterState, RouterStateModel } from './router.state';
export { ROUTER_STATE_TOKEN, RouterState, RouterStateModel } from './router.state';
export {
RouterStateSerializer,
DefaultRouterStateSerializer,
Expand Down
25 changes: 17 additions & 8 deletions packages/router-plugin/src/router.state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
NavigationEnd,
Event
} from '@angular/router';
import { Action, Selector, State, StateContext, Store } from '@ngxs/store';
import { Action, Selector, State, StateContext, StateToken, Store } from '@ngxs/store';
import {
NavigationActionTiming,
NgxsRouterPluginOptions,
Expand Down Expand Up @@ -44,8 +44,13 @@ export type RouterTrigger =
// The `devtools` trigger means that the state change has been triggered by Redux DevTools (e.g. when the time-traveling is used).
| 'devtools';

// NGXS doesn't permit untyped selectors, such as `select(RouterState)`,
// as the `RouterState` class itself lacks type information. Therefore,
// the following state token must replace `RouterState`.
export const ROUTER_STATE_TOKEN = new StateToken<RouterStateModel>('router');

@State<RouterStateModel>({
name: 'router',
name: ROUTER_STATE_TOKEN,
defaults: {
state: undefined,
navigationId: undefined,
Expand Down Expand Up @@ -78,12 +83,14 @@ export class RouterState implements OnDestroy {

@Selector()
static state<T = RouterStateSnapshot>(state: RouterStateModel<T>) {
return state && state.state;
// The `state` is optional if the selector is invoked before the router
// state is registered in NGXS.
return state?.state;
}

@Selector()
static url(state: RouterStateModel): string | undefined {
return state && state.state && state.state.url;
return state?.state?.url;
}

constructor(
Expand Down Expand Up @@ -134,15 +141,17 @@ export class RouterState implements OnDestroy {
}

private _setUpStoreListener(): void {
const routerState$ = this._store.select(RouterState).pipe(takeUntil(this._destroy$));
const routerState$ = this._store
.select(ROUTER_STATE_TOKEN)
.pipe(takeUntil(this._destroy$));
routerState$.subscribe((state: RouterStateModel | undefined) => {
this._navigateIfNeeded(state);
});
}

private _navigateIfNeeded(routerState: RouterStateModel | undefined): void {
if (routerState && routerState.trigger === 'devtools') {
this._storeState = this._store.selectSnapshot(RouterState);
this._storeState = this._store.selectSnapshot(ROUTER_STATE_TOKEN);
}

const canSkipNavigation =
Expand All @@ -157,7 +166,7 @@ export class RouterState implements OnDestroy {
return;
}

this._storeState = this._store.selectSnapshot(RouterState);
this._storeState = this._store.selectSnapshot(ROUTER_STATE_TOKEN);
this._trigger = 'store';
this._ngZone.run(() => this._router.navigateByUrl(this._storeState!.state!.url));
}
Expand Down Expand Up @@ -205,7 +214,7 @@ export class RouterState implements OnDestroy {
this._routerState = this._serializer.serialize(this._router.routerState.snapshot);

if (this._trigger !== 'none') {
this._storeState = this._store.selectSnapshot(RouterState);
this._storeState = this._store.selectSnapshot(ROUTER_STATE_TOKEN);
this._dispatchRouterAction(new RouterRequest(this._routerState, event, this._trigger));
}
}
Expand Down
23 changes: 14 additions & 9 deletions packages/router-plugin/tests/router-data-resolved.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,9 @@ describe('RouterDataResolved', () => {
const dataFromTheOriginalRouter = router.routerState.snapshot.root.firstChild!.data;
expect(dataFromTheOriginalRouter).toEqual({ test });

const dataFromTheRouterState = store.selectSnapshot(RouterState.state)!.root.firstChild!
.data;
const dataFromTheRouterState = store.selectSnapshot<RouterStateSnapshot | undefined>(
RouterState.state
)!.root.firstChild!.data;
expect(dataFromTheOriginalRouter).toEqual(dataFromTheRouterState);
})
);
Expand Down Expand Up @@ -156,8 +157,9 @@ describe('RouterDataResolved', () => {
const dataFromTheOriginalRouter = router.routerState.snapshot.root.firstChild!.data;
expect(dataFromTheOriginalRouter).toEqual({ test });

const dataFromTheRouterState = store.selectSnapshot(RouterState.state)!.root.firstChild!
.data;
const dataFromTheRouterState = store.selectSnapshot<RouterStateSnapshot | undefined>(
RouterState.state
)!.root.firstChild!.data;
expect(dataFromTheOriginalRouter).toEqual(dataFromTheRouterState);
})
);
Expand Down Expand Up @@ -215,6 +217,11 @@ describe('RouterDataResolved', () => {
})
@Injectable()
class CounterState {
@Selector()
static getState(state: number) {
return state;
}

@Action(RouterNavigation)
routerNavigation(ctx: StateContext<number>): void {
ctx.setState(ctx.getState() + 1);
Expand All @@ -233,7 +240,7 @@ describe('RouterDataResolved', () => {
await ngZone.run(() => router.navigateByUrl('/a/b'));

// Assert
const counter = store.selectSnapshot<number>(CounterState);
const counter = store.selectSnapshot<number>(CounterState.getState);
expect(counter).toEqual(3);
})
);
Expand Down Expand Up @@ -272,10 +279,8 @@ describe('RouterDataResolved', () => {

const subscription = store.select(CounterState.counter).subscribe();

await ngZone.run(async () => {
await router.navigateByUrl('/a/b/c');
await router.navigateByUrl('/a/b');
});
await ngZone.run(() => router.navigateByUrl('/a/b/c'));
await ngZone.run(() => router.navigateByUrl('/a/b'));
subscription.unsubscribe();

// Assert
Expand Down
20 changes: 5 additions & 15 deletions packages/store/src/store.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { Inject, Injectable, Optional, Signal, Type, computed } from '@angular/core';
import { Inject, Injectable, Optional, Signal, computed } from '@angular/core';
import { Observable, of, Subscription, throwError } from 'rxjs';
import { catchError, distinctUntilChanged, map, shareReplay, take } from 'rxjs/operators';
import { ɵINITIAL_STATE_TOKEN, ɵPlainObject, StateToken } from '@ngxs/store/internals';
import { ɵINITIAL_STATE_TOKEN, ɵPlainObject } from '@ngxs/store/internals';

import { InternalNgxsExecutionStrategy } from './execution/internal-ngxs-execution-strategy';
import { InternalStateOperations } from './internal/state-operations';
Expand Down Expand Up @@ -47,10 +47,7 @@ export class Store {
/**
* Selects a slice of data from the store.
*/
select<T>(selector: (state: any, ...states: any[]) => T): Observable<T>;
select<T = any>(selector: string | Type<any>): Observable<T>;
select<T>(selector: StateToken<T>): Observable<T>;
select(selector: any): Observable<any> {
select<T>(selector: TypedSelector<T>): Observable<T> {
const selectorFn = this.getStoreBoundSelectorFn(selector);
return this._selectableStateStream.pipe(
map(selectorFn),
Expand All @@ -71,21 +68,14 @@ export class Store {
/**
* Select one slice of data from the store.
*/

selectOnce<T>(selector: (state: any, ...states: any[]) => T): Observable<T>;
selectOnce<T = any>(selector: string | Type<any>): Observable<T>;
selectOnce<T>(selector: StateToken<T>): Observable<T>;
selectOnce(selector: any): Observable<any> {
selectOnce<T>(selector: TypedSelector<T>): Observable<T> {
return this.select(selector).pipe(take(1));
}

/**
* Select a snapshot from the state.
*/
selectSnapshot<T>(selector: (state: any, ...states: any[]) => T): T;
selectSnapshot<T = any>(selector: string | Type<any>): T;
selectSnapshot<T>(selector: StateToken<T>): T;
selectSnapshot(selector: any): any {
selectSnapshot<T>(selector: TypedSelector<T>): T {
const selectorFn = this.getStoreBoundSelectorFn(selector);
return selectorFn(this._stateStream.getValue());
}
Expand Down
7 changes: 0 additions & 7 deletions packages/store/types/tests/selection.lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,26 +57,19 @@ describe('[TEST]: Action Types', () => {
}
}

assertType(() => store.select(TodoState)); // $ExpectType Observable<any>
assertType(() => store.select(TodoState.getTodo)); // $ExpectType Observable<string[]>
assertType(() => store.select<string[]>(TodoState)); // $ExpectType Observable<string[]>
assertType(() => store.select('state.value')); // $ExpectType Observable<any>
assertType(() => store.select(state => state.foo.bar)); // $ExpectType Observable<any>
assertType(() => store.select({ foo: 'bar' })); // $ExpectError
assertType(() => store.select()); // $ExpectError

assertType(() => store.selectOnce(TodoState)); // $ExpectType Observable<any>
assertType(() => store.selectOnce(TodoState.getTodo)); // $ExpectType Observable<string[]>
assertType(() => store.selectOnce<string[]>(TodoState)); // $ExpectType Observable<string[]>
assertType(() => store.selectOnce('state.value')); // $ExpectType Observable<any>
assertType(() => store.selectOnce(state => state.foo.bar)); // $ExpectType Observable<any>
assertType(() => store.selectOnce({ foo: 'bar' })); // $ExpectError
assertType(() => store.selectOnce()); // $ExpectError

assertType(() => store.selectSnapshot(TodoState)); // $ExpectType any
assertType(() => store.selectSnapshot(TodoState.getTodo)); // $ExpectType string[]
assertType(() => store.selectSnapshot<string[]>(TodoState)); // $ExpectType string[]
assertType(() => store.selectSnapshot('state.value')); // $ExpectType any
assertType(() => store.selectSnapshot(state => state.foo.bar)); // $ExpectType any
assertType(() => store.selectSnapshot({ foo: 'bar' })); // $ExpectError
assertType(() => store.selectSnapshot()); // $ExpectError
Expand Down
6 changes: 0 additions & 6 deletions packages/store/types/tests/state-token.lint.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,21 +181,15 @@ describe('[TEST]: StateToken', () => {
.componentInstance;

const snapshotByToken = appComponent.store.selectSnapshot(FOO_TOKEN);
const snapshotByStateClass = appComponent.store.selectSnapshot(FooState);

assertType(() => snapshotByToken); // $ExpectType MyModel
assertType(() => snapshotByStateClass); // $ExpectType any

const selectByToken = appComponent.store.select(FOO_TOKEN);
const selectByStateClass = appComponent.store.select(FooState);

assertType(() => selectByToken); // $ExpectType Observable<MyModel>
assertType(() => selectByStateClass); // $ExpectType Observable<any>

const selectOnceByToken = appComponent.store.selectOnce(FOO_TOKEN);
const selectOnceByStateClass = appComponent.store.selectOnce(FooState);

assertType(() => selectOnceByToken); // $ExpectType Observable<MyModel>
assertType(() => selectOnceByStateClass); // $ExpectType Observable<any>
});
});

0 comments on commit fc8d954

Please sign in to comment.