Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(dispatch-data-to-store): add function overloads and allow SSOT by using selector #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat(dispatch-data-to-store): add function overloads and allow SSOT by using selector #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 20 additions & 9 deletions apps/store-test/src/app/app.component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { Component } from '@angular/core';
import { Store } from '@ngrx/store';
import { tap } from 'rxjs';
import { actions, selectors } from '../store/user.store';
import { CoursesService } from '../services/courses.service';

Expand All @@ -20,20 +19,32 @@ export class AppComponent {
this.store.dispatch(actions.users.effects.add({ payload: 'serguey' }));
this.store.dispatch(actions.paging.effects.set());

this.store.select(selectors.admins.selectAll).pipe(tap(console.log)).subscribe();
this.store.select(selectors.admins.selectAll).subscribe((result) => {
console.log('Admins: ', result);
});

this.courseService.state.completed$.subscribe((result) => {
console.log('Completed ', result);
});

this.courseService.state.coursesLoading$.subscribe((result) => {
console.log('Loading ', result);
});

this.courseService.state.courses$.subscribe((result) => {
console.log('Courses ', result);
});

this.courseService.state.completed$.subscribe(console.log);
this.courseService.state.coursesLoading$.subscribe((result) =>
console.log('Loading ', result)
);
this.courseService.state.courses$.subscribe((result) => console.log('Courses ', result));
this.courseService.state.amount$.subscribe((result) => {
console.log('Amount ', result);
});

setTimeout(() => {
this.courseService.setCompleted();
this.courseService.dispatchCourses().subscribe();
}, 5000);
this.courseService.dispatchCourses().subscribe((courses) => {
// Wouter: should be undefined because no selector was provided in the switchmap
console.log('Amount of courses after delay: ', courses);
});
}, 1200);
}
}
20 changes: 17 additions & 3 deletions apps/store-test/src/services/courses.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Injectable } from '@angular/core';
import { Store } from '@ngrx/store';
import { of } from 'rxjs';
import { Observable, of, switchMap } from 'rxjs';
import { CoursesStore, actions, selectors } from '../store/courses.store';
import { StoreService, dispatchDataToStore } from '@ngx/store';

Expand All @@ -14,7 +14,21 @@ export class CoursesService extends StoreService<CoursesStore> {
this.store.dispatch(actions.completed.set({ payload: true }));
}

dispatchCourses() {
return dispatchDataToStore(actions.courses, of(['hello', 'world']), this.store);
dispatchCourses(): Observable<number> {
return dispatchDataToStore(
actions.courses,
selectors.courses,
of(['hello', 'world']),
this.store
).pipe(
switchMap((courses) => {
return dispatchDataToStore(
actions.amount,
undefined,
of(courses.length),
this.store
);
})
);
}
}
152 changes: 137 additions & 15 deletions libs/store/src/lib/store/utils/dispatch-data-to-store/dispatch-data-to-store.util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,37 +1,159 @@
import { Store } from '@ngrx/store';
import { Observable, throwError } from 'rxjs';
import { catchError, finalize, tap } from 'rxjs/operators';
import { Observable, of, throwError } from 'rxjs';
import { catchError, finalize, switchMap, tap } from 'rxjs/operators';
import {
BaseStoreActions,
BaseStoreSelectors,
EntityStoreActions,
EntityStoreSelectors,
} from '../../interfaces';

// -------------------
// Overloads
// -------------------
// BASE STORE
export function dispatchDataToStore<DataType = any>(
dispatchAction: BaseStoreActions<DataType>,
selectAction: BaseStoreSelectors<DataType>,
data: Observable<DataType>,
store: Store,
dispatchType: 'set'
): Observable<DataType>;
export function dispatchDataToStore<DataType = any>(
dispatchAction: BaseStoreActions<DataType>,
selectAction: BaseStoreSelectors<DataType>,
data: Observable<DataType>,
store: Store
): Observable<DataType>;
// NO SELECTOR
export function dispatchDataToStore<DataType = any>(
dispatchAction: BaseStoreActions<DataType>,
selectAction: null | undefined,
data: Observable<DataType>,
store: Store,
dispatchType: 'set'
): Observable<null>;
export function dispatchDataToStore<DataType = any>(
dispatchAction: BaseStoreActions<DataType>,
selectAction: null | undefined,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not that familiar with these overloads, but what will be the effect of putting the selectAction here on the existing implementation?

It's one thing that the result of this PR forces you to change the return types, but completely changing the order of this implementation forcing you to pass undefined as a second parameter would make it a lot harder to work wit.

I'd personally expect the overload to be either with the dispatchAction and one without it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that would be possible, I presume! 👍

data: Observable<DataType>,
store: Store
): Observable<null>;
// ENTITY STORE
export function dispatchDataToStore<DataType = any>(
dispatchAction: EntityStoreActions<DataType>,
selectAction: EntityStoreSelectors<DataType>,
data: Observable<DataType[]>,
store: Store,
dispatchType: 'set' | 'add' | 'update'
): Observable<DataType[]>;
export function dispatchDataToStore<DataType = any>(
dispatchAction: EntityStoreActions<DataType>,
selectAction: EntityStoreSelectors<DataType>,
data: Observable<DataType[]>,
store: Store
): Observable<DataType[]>;
// NO SELECTOR
export function dispatchDataToStore<DataType = any>(
dispatchAction: EntityStoreActions<DataType>,
selectAction: null | undefined,
data: Observable<DataType[]>,
store: Store,
dispatchType: 'set' | 'add' | 'update'
): Observable<null>;
export function dispatchDataToStore<DataType = any>(
dispatchAction: EntityStoreActions<DataType>,
selectAction: null | undefined,
data: Observable<DataType[]>,
store: Store
): Observable<null>;

// -------------------
// Definition
// -------------------
/**
* Dispatches data to the store based on a provided action and Observable. Loading and error state will be handled by default.
*
* @param dispatchAction - The action we wish to use to dispatch data to the store
* @param selectAction - The selector we wish to use to select the data from the store. If not provided, `null` will be returned.
* @param data - The data we wish to dispatch to the store
* @param store - The store we wish to dispatch the data to
* @param dispatchType - Whether we wish to set, add or update the data. By default this is `set`
* @return {*} {Observable<DataType>}
* @returns {Observable<(DataType | DataType[] | null)>} - Returns the data that was dispatched to the store. The generic type is the same as the `data`.
*/
export const dispatchDataToStore = <DataType>(
dispatchAction: any,
data: Observable<DataType>,
export function dispatchDataToStore<DataType = any>(
dispatchAction: BaseStoreActions<DataType> | EntityStoreActions<DataType>,
selectAction: BaseStoreSelectors<DataType> | EntityStoreSelectors<DataType> | undefined,
data: Observable<DataType | DataType[]>,
store: Store,
dispatchType: 'set' | 'add' | 'update' = 'set'
): Observable<DataType> => {
): Observable<DataType | DataType[]> | null {
// Iben: Set the loading state to true and the error state to false to start a new set
store.dispatch(dispatchAction.loading({ payload: true }));
store.dispatch(dispatchAction.error({ payload: false }));

// Iben: Set, add or update the data according to the provided dispatch type
return data.pipe(
tap((payload) => {
if (dispatchType === 'set') {
store.dispatch(dispatchAction.set({ payload }));
} else if (dispatchType === 'add') {
store.dispatch(dispatchAction.add({ payload }));
} else {
store.dispatch(dispatchAction.update({ payload }));
switch (dispatchType) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that a switch case or an if else structure in this case makes no difference, I'd keep it the way it is if it isn't necessary to change it.

Changing these kinds of things in a PR without real cause makes it harder to review PRs, because you have to first try to figure out why it was done in relation to the changes, and then realize it wasn't really needed to do so.

case 'set': {
return payload instanceof Array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're making too big of an assumption here. Yes, ngrx strongly suggests using the entity store for arrays, but it's not mandatory. So people are still allowed to use the regular approach to use arrays. This package facilitates the process of ngrx, so we cannot prevent people from using it like that.

These checks will have to go, because they're too strongly set.

? store.dispatch(
(dispatchAction as EntityStoreActions<DataType>).set({
payload,
})
)
: store.dispatch(
(dispatchAction as BaseStoreActions<DataType>).set({
payload,
})
);
}
case 'add': {
if (!(payload instanceof Array)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is also not really correct. If you would have an array of arrays in your store, you can no longer use the add method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so? An array of an array is still of type Array. Since we negate the result and then throw an error, the user is forced to provide an array when using add. An array of arrays would most definitely pass this check and be dispatched to the store. 🙂

[[]] instanceof Array // true

The type BaseStoreAsset also does not support the add method, hence the reason for this aggressive error throwing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I don't remember what I was referring to tbh 😅

return throwError(
() =>
new Error(
'NgxStore: Payload must be an array when using "add". Also make sure your storeslice is of type "EntityStoreAssets".'
)
);
}

return store.dispatch(
(dispatchAction as EntityStoreActions<DataType>).add({ payload })
);
}
case 'update': {
if (payload instanceof Array) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this check

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps I am incorrect here, but shouldn't the payload of the update not be an Array? (sorry, I can only reference 1 line, apparently 🤷)

ngx-tools/libs/store/src/lib/store/interfaces/entity-store-assets.ts

Line 42 in 36b311d

update: ActionCreator<

Type BaseStoreAsset also does not support an update, I presume there is a reason that was initially left out of its operators?

return throwError(
() =>
new Error(
'NgxStore: Payload must not be an array when using "update".'
)
);
}

return store.dispatch(
(dispatchAction as EntityStoreActions<DataType>).update({
payload,
})
);
}
}
}),
switchMap((payload) => {
if (!selectAction) {
return of(null);
}

// Wouter: Get the value recently set to the store. This is done to keep the Store as SSOT.
return store.select(
payload instanceof Array
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to run a check on the selectAction rather than on the payload, because again, that's not possible with how the store works.

You'll be better off doing something like selectAction['selectAll'] ? selectAction.selectAll : selectAction.select

? (selectAction as EntityStoreSelectors).selectAll
: (selectAction as BaseStoreSelectors).select
);
}),
// Iben: Catch the error and dispatch it to the store
// Iben: Catch the error and dispatch its to the store
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why its? 😅 You want to dispatch the error to the store, aka it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, no idea how that got changed! 😅

catchError((err) => {
store.dispatch(dispatchAction.error({ payload: err }));

Expand All @@ -42,4 +164,4 @@ export const dispatchDataToStore = <DataType>(
store.dispatch(dispatchAction.loading({ payload: false }));
})
);
};
}