-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
feat(dispatch-data-to-store): add function overloads and allow SSOT by using selector #165
base: master
Are you sure you want to change the base?
Conversation
store.dispatch(dispatchAction.update({ payload })); | ||
switch (dispatchType) { | ||
case 'set': { | ||
return payload instanceof Array |
There was a problem hiding this comment.
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.
); | ||
} | ||
case 'add': { | ||
if (!(payload instanceof Array)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😅
|
||
// Wouter: Get the value recently set to the store. This is done to keep the Store as SSOT. | ||
return store.select( | ||
payload instanceof Array |
There was a problem hiding this comment.
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
}), | ||
// Iben: Catch the error and dispatch it to the store | ||
// Iben: Catch the error and dispatch its to the store |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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! 😅
); | ||
} | ||
case 'update': { | ||
if (payload instanceof Array) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this check
There was a problem hiding this comment.
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 🤷)
update: ActionCreator< |
Type BaseStoreAsset
also does not support an update
, I presume there is a reason that was initially left out of its operators?
store.dispatch(dispatchAction.add({ payload })); | ||
} else { | ||
store.dispatch(dispatchAction.update({ payload })); | ||
switch (dispatchType) { |
There was a problem hiding this comment.
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.
): Observable<null>; | ||
export function dispatchDataToStore<DataType = any>( | ||
dispatchAction: BaseStoreActions<DataType>, | ||
selectAction: null | undefined, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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! 👍
To address the comments regarding the errors that are thrown here in the dispatchDataToStore. I've checked but basically came to the conclusion that all of these errors that are thrown are unnecessary.
In summary, these errors are not needed and if we want to improve the behaviour here, it should be through typing first and foremost. |
089a236
to
dea1c39
Compare
Explanation
The SSOT principle was not properly respected. The
dispatchDataToStore
util will not return anything in and of itself, a selector has to be provided. If not provided, the method will returnObservable<null>
.Examples
The function overloads make sure that something like this will not be allowed:
The error will indicate that no contract of any overload matches. The
amount
is of type number, which indicates aBaseStoreAsset<number>
, while the data provided is of typeArray
. The overloads will prevent this from passing, whereas previously this would fail to set the data in the store.The error looks like this:
The function overloads also make sure a
BaseStoreAsset
cannot haveupdate
oradd
as dispatchType, for they are only available onEntityStoreAssets
.Possible improvements
Moving the function overloads to a type definition file
dispatch-data-to-store.util.d.ts
, for example.