-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix(data): make non-optimistic add command entity partial #3859
fix(data): make non-optimistic add command entity partial #3859
Conversation
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
@@ -11,6 +11,10 @@ export interface EntityServerCommands<T> { | |||
* @returns A terminating Observable of the entity | |||
* after server reports successful save or the save error. | |||
*/ | |||
add( | |||
entity: Partial<T>, | |||
options: EntityActionOptions & { isOptimistic: false } |
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.
Could you also add a test that verifies this functionality please?
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 this is just a type change, I found it not necessary. I will add a test that would cause Typescript to fail build if it does not work.
Digging a little deeper in the dispatcher I found out that the default behavior is indeed pessimistic. However, the initial PR types have been reverted, and omitting the options argument causes a non-partial entity argument (optimistic-like). I suggest using add(
entity: Partial<T>,
options?: EntityActionOptions & { isOptimistic?: false }
): Observable<T>;
add(
entity: T,
options: EntityActionOptions & { isOptimistic: true }
): Observable<T>;
add(entity: T, options?: EntityActionOptions): Observable<T>; which seems to cover all cases without fail. |
Mirror the type definition of EntityCollectionServiceBase.add on EntityServerCommands.add. Closes EntityServerCommands.add does not allow optional id #3847
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.
👍 thanks
Mirror the type definition of EntityCollectionServiceBase.add on EntityServerCommands.add.
Closes EntityServerCommands.add does not allow optional id #3847
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
EntityServerCommands.add()
allows only full entities.Closes #3847
What is the new behavior?
EntityServerCommands.add()
allows partial entities if non-optimistic, just likeEntityCollectionServiceBase.add()
.Does this PR introduce a breaking change?