UpsertOne does not clear missing optional fields

[tkglaser]

Minimal reproduction of the bug/regression with instructions:

https://stackblitz.com/edit/ngrx-seed-eichyx

Press the buttons in the order of WITHOUT OPTIONAL -> WITH OPTIONAL -> WITHOUT OPTIONAL. Notice, that once the optional field is set, it is never unset.

Expected behavior:

I would expect that upsertOne() replaces the object in the store with the object I've passed in. Instead it seems to merge the two together. This way when an optional field is missing in the update, that field stays in the store forever. I find this counter intuitive.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

ngrx 6.4, NG 7, Node 10, Chrome, WIndows 10

Other information:

I would be willing to submit a PR to fix this issue

[X] Yes (Assistance is provided if you need help submitting a pull request)
[ ] No

[timdeschryver]

At first, I was thinking that this isn't a bug. But after giving it some thought I do agree because it has the entity: T, state: R signature, thus it should just replace the entity in the store. Would you agree with this @brandonroberts ?

Instead it seems to merge the two together

Indeed, the two objects are being merged - ./modules/entity/src/unsorted_state_adapter.ts#L100

To clear it you will have to assign it to for example an empty string, null or undefined.

[brandonroberts]

Its not a bug, but I think it is a preference. To me an upsert should be a non-destructive update (unless you make fields null/undefined) or insert if not present. Its currently geared towards partial updates, which I think is correct.

[timdeschryver]

Those were my initial thoughts 😄

I think we can close this issue?

[brandonroberts]

Yep

[tkglaser]

I see what you're saying and I agree it's a matter of preference. To me, intuitively, an upsert should be a full replace otherwise it's the same as an update. However, the workaround is straightforward, so it's not that big of a deal. Ngrx is a great package and I'd just like to take a moment to thank you guys for maintaining it and doing such a great job in general! 🙂

[antonyRoberts]

@tkglaser The generally accepted term for upsert is to insert a new entry if none exists, update the entry if it does.

[tkglaser]

@antonyRoberts I know. It's the signature of the function that threw me. As Tim pointed out above, it takes a T and not a Partial<T> which led me to believe, the whole thing gets replaced. From a semantic point of view it should really be T | Partial<T>, which is of course a type definition that doesn't make much sense because T is already a Partial<T>. As I said above, nevermind, the solution is straightforward.

[BobobUnicorn]

@brandonroberts Can this be explicitly documented? It's rather unexpected behaviour, especially given that the signature does nothing to imply it will merge the provided objects with existing state.

[alex-okrushko]

FYI, for anyone looking for solution, you can explicitly set the fields with undefined to erase them with upsert, here is the adjusted demo: https://stackblitz.com/edit/ngrx-seed-yxhcjm?file=src/app/app.component.ts