Better typings for dataPersistence

[rafaelss95]

Problems

1. state on dataPersistence callbacks is nullable.

nx/packages/angular/src/runtime/nx/data-persistence.ts

Lines 36 to 40 in 8e519cd

	export interface FetchOpts<T, A> {
	id?(a: A, state?: T): any;
	run(a: A, state?: T): Observable<Action> | Action | void;
	onError?(a: A, e: any): Observable<any> | any;
	}

nx/packages/angular/src/runtime/nx/data-persistence.ts

Lines 75 to 76 in 8e519cd

	export function fetch<T, A extends Action>(opts: FetchOpts<T, A>) {
	return (source: ActionStateStream<T, A>): Observable<Action> => {

With strict type enabled, we can't use the state directly without checking its nullability before. Ex:

readonly test$ = createEffect(() =>
  this.actions$.pipe(
    concatMap(action => of(action).pipe(
      withLatestFrom(this.store$.select(someSelector)),
    ),
    navigation(AComponent, {
      run: (_, state) => {
        state.awesome // Type error as state is nullable
      },
    }),
  ),
);

2. Conditional return:

• If it's a non-dispatching effect then it should be void;
• If it's a dispatching effect then it could be either Action or Observable<Action>.

Note that currently I can write something like this:

readonly test$ = createEffect(() =>
  this.actions$.pipe(
    navigation(AComponent, {
      run: () => {
        someAction(); // missing return intentionally
      },
    }),
  ),
);

... without type errors :(. It would be awesome if we conditionally defined the correct type to avoid mistakes like this.

3. In documentation you say:

nx/packages/angular/src/runtime/nx/data-persistence.ts

Lines 306 to 307 in 8e519cd

	* `fetch` addresses these problems--it runs all fetches in order, which removes race conditions
	* and forces the developer to handle errors.

"[...] and forces the developer to handle errors [...]"

... however the onError is optional. Maybe I just misunderstood the sentence?

4. Change from any to unknown in onError, in order to avoid some typos, while accessing props. Somewhat related to Error in catch clause should be unknown microsoft/TypeScript#26174.

Suggestions

For the points 1, 3 and 4, I got this idea (not sure if it's the best way, but...):

export interface FetchOpts<T, A> { 
-  id?(a: A, state?: T): any; 
+  id?(a: A, state: T): unknown;
-  run(a: A, state?: T): Observable<Action> | Action | void; 
+  run(a: A, state: T): Observable<Action> | Action | void;
-  onError?(a: A, e: any): Observable<any> | any; 
+  onError(a: A, e: unknown): unknown;
 } 

Environment

Angular 10
Ngrx 10
NX 10

[brandonroberts]

@rafaelss95 better typings would be an improvement. Would you like to put in a PR for this?

[github-actions]

This issue has been automatically marked as stale because it hasn't had any recent activity. It will be closed in 14 days if no further activity occurs.
If we missed this issue please reply to keep it active.
Thanks for being a part of the Nx community! 🙏

[github-actions]

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.