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.

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

Update and upsert functions are not taking into account removed properties #2061

Closed
maxime1992 opened this issue Aug 12, 2019 · 4 comments
Closed

Comments

@maxime1992
Copy link
Contributor

Minimal reproduction of the bug/regression with instructions:

I have an object that has some optional fields.
Let say that the interface looks like the following:

interface MyObject {
  propA: string;
  propB?: string;
}

If at a given point in time the object is set in the store with both properties and I want to remove the optional one later, I can (using update or upsert).

CF this line:

const updated: T = Object.assign({}, original, update.changes);

The repro doesn't need a full ngrx repro, following should be enough:

Object.assign(
  {},
  { propA: 'a original', propB: 'B original' },
  { propA: 'a updated and optional B is not needed anymore!' } // note that I'm trying to remove propB here!
);

Output:

{ propA: "a updated and optional B is not needed anymore!", propB: "B original" }

Expected behavior:

Output should be:

{ propA: "a updated and optional B is not needed anymore!" }

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

Ngrx 8.2.0

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

I could work on a PR if I'm not the only one considering the current behavior as a bug :)

@manklu
Copy link

manklu commented Aug 12, 2019

see #1604

@manklu
Copy link

manklu commented Aug 13, 2019

@maxime1992 Issue #1604 contains the answer for upsert, work as designed.

And since upsert is a shortcut for insert or update, you have your answer for update too.

@maxime1992
Copy link
Contributor Author

Indeed. Feels like passing an option to the method might be a good think rather than having to set all the fields to remove to null or undefined though.

@timdeschryver
Copy link
Member

Closing in favor of #2369

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants