fix(data): make options optional on add with partial

[donohoea]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Closes #3041

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[ngrxbot]

Preview docs changes for 34522c3 at https://previews.ngrx.io/pr3043-34522c36/

[donohoea]

@yharaskrik maybe the intention was to enforce explicitly adding the isOptimistic: false option to an add with a partial and I'm wrong with the fix I've provided. Bu it does seem redundant to me to require the options with at least the isOptimistic: false when add() defaults to isOptimistic false. Open to your thoughts.

[yharaskrik]

I think I may have messed up, my intention was to say: If options are not provided || isOptimistic: false then T is required otherwise if isOptimistic: true then Partial<T> is required. Let me know if that was the case and I made a mistake originally.

[donohoea]

@yharaskrik what I see is if you provide T options are optional and no restrictions are place on the value of isOptimistic. If Partial<T> is provided options are not optional and at least isOptimistic: false needs to be provided.

Looking back a few commits it looks like your intent was to add the ability to use a Partial<T> in the call to add, specifically on an pessimistic add where the server is going to provide the id of the entity. Though you did also mention the case where other fields might be better set on the server, which could be equally applicable in an optimistic add and pessimistic add. So requiring the isOptimistic: false flag be set with a Partial<T> when isOptimistic: false is already the default option and also a Partial<T> could be useful on an optimistic add, seems wrong to me. I would suggest the signature add(entity: Partial<T>, options?: EntityActionOptions): Observable<T>. This would allow an optimistic save without and id field but the entity action guard would throw an error on execution.

A bit late to the game here but the original signature of add(entity: T, options?: EntityActionOptions): Observable<T> allowed:

interface Thing {
  id?: string
  name: string
  createdTimeStamp?: number
}

thingEntityCollectionService.add({name: 'this thing'})

this required you to be explicit about which fields were optional and simplified the signature with no Partial<T> signature, was this not a workable solution for you?

@e-oz was the issue you ran into an existing piece of code that was working in an earlier version that stopped working on upgrade. Or new code written after upgrading to the latest version?

[e-oz]

@donohoea old (and not even mine) code refused to compile.

[timdeschryver]

Won't this change undo that desired behavior that @yharaskrik added? And make it worde because we allow a Partia<T> now?
I think the problem here, is that TS doesn't see this as an overload.
What about adding another overload where options is optional and T can't be a partial (the original implementation)?

  add(
    entity: Partial<T> | T,
    options: EntityActionOptions & { isOptimistic: false }
  ): Observable<T>;
  add(entity: T, options?: EntityActionOptions): Observable<T>;
  add(entity: T, options?: EntityActionOptions): Observable<T> {
    return this.dispatcher.add(entity, options);
  }

That way, you can either provide them, leave them out, and when optimistic is false then T is a partial.

[donohoea]

You're right @timdeschryver I was mistakenly seeing the implementation as an overload. My only issue with the solution you suggested is that my expectation as a user would be that this would be acceptable:

this.storyDataService.add(story as Partial<Story>)

Given that isOtimistic: false is the default. But that's not the issue we're trying to solve here and is I believe the strict typing @yharaskrik was aiming for and was accepted earlier so I'll update the PR.

[donohoea]

seems like my updates invalidated nx caching on the data-example-app:build and exposed a pre-exisitng issue. Switched to master and nx run data-example-app:build succeeds from nx cache but nx run data-example-app:build --skip-nx-cache fails.
See #3054.

[yharaskrik]

Hey guys, sorry it's been a wild week for me. My intention was to allow a partial to be passed in if optimistic was false but if it was true (or left out, so true by default) then it was not partial. I know some ideas here have been thrown out due to TS not picking it up as an overload. Let me know if you guys need my input on this or if you have it figured out.

In regards to the code using some old cache and not compiling. I have no idea how that could have happened and would love to know to prevent it in the future. It all seemed to work last time I spun it up! (A while ago though.)

[e-oz]

@yharaskrik it has nothing to do with the cache. Parameter was optional, now it's required - code can’t be compiled, if parameter is not set. As you see - there is no any old cache.

[yharaskrik]

seems like my updates invalidated nx caching on the data-example-app:build and exposed a pre-exisitng issue. Switched to master and nx run data-example-app:build succeeds from nx cache but nx run data-example-app:build --skip-nx-cache fails.
See #3054.

@e-oz this is what I was referring to. I understand the parameter was setup wrong. I must have made a mistake I just don't know how it passed everything if it wasn't able to be compiled.

[donohoea]

@yharaskrik I don't think the build error is related to your code or mine, all builds work for me after deleting node_modules/@ngrx but that doesn't help with the ci checks (so I've opened a separate issue). But I think you may have one of your assumptions wrong:

My intention was to allow a partial to be passed in if optimistic was false but if it was true (or left out, so true by default) then it was not partial.

isOptimistic is actually false by default (unless you're providing your own EntityDispatcherDefaultOptions). Which makes me feel like I should be able to pass a partial without explicitly setting isOptimistic: false (as it is the default). This PR currently does not address that bit but should solve the unintended breaking change that does not allow an add with a non-partial and no options.

[timdeschryver]

@donohoea can you rebase to please?
It should contain the fix for the build error.

[timdeschryver]

LGTM, thanks!

[e-oz]

Works great with 12.2, thanks!