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

ngOnChanges produces changes for every input, even when the objects are unchanged. #403

Closed
Jrubzjeknf opened this issue Oct 14, 2020 · 6 comments · Fixed by #404
Closed

Comments

@Jrubzjeknf
Copy link

Problem: ngOnChanges seems to always provide all inputs in the SimpleChanges object, even though some inputs are unchanged. Correct behavior would be that only the changed inputs are provided, similar to the Angular implementation.

Repro steps:

  • Open stackblitz
  • Click on val1 button, which changes the someValue property on HelloComponent.
  • Notice in the console that the following SimpleChanges object is generated. The issue is that name should not come up as a change, since it has not changed.
{
  "name": {
    "previousValue": "World",
    "currentValue": "World",
    "firstChange": false
  },
  "someValue": {
    "previousValue": "myvalue",
    "currentValue": "val1",
    "firstChange": false
  }
}

My own debugging shows that all bound input properties are always marked as changed. I can work around it for now by checking for equality myself in the ngOnChanges method of my component, but a fix would of course be preferable.

@gund
Copy link
Owner

gund commented Oct 14, 2020

Thanks for reporting this issue!
It does look like it's creating changes for unchanged inputs which is super weird as it did not do it before...

I will investigate what is the cause and fix it soon.

@gund gund added the bug label Oct 14, 2020
@gund gund self-assigned this Oct 14, 2020
@gund
Copy link
Owner

gund commented Oct 14, 2020

Okay so after my investigation I figured out that this is a regression in Angular's KeyValueDiffer Service that this library uses in order to figure out the changes in inputs - it basically reports all properties to be changed even if just one property in object was changed...

I'm not sure if this was a change by design or a regression in Angular so I'm going to open an issue there and ask for clarification.

After their response we will know how to address this bug but for now I would recommend you to simply do a reference checks in OnChanges hook manually.

@gund
Copy link
Owner

gund commented Oct 14, 2020

During deeper investigation is turns out that Angular's differ works fine and library was using incorrect API to collect changes from it.

Currently new records are collected from forEachAddedItem() method and changed records from forEachItem() method which always returns all items despite any changes (which is logical).
See:

private _collectChangesFromDiffer(differ: KeyValueChangesAny): SimpleChanges {
const changes = {} as SimpleChanges;
differ.forEachAddedItem(recordToChanges(changes));
differ.forEachItem(recordToNewChanges(changes));

But there is another method forEachChangedItem() that returns only changed records and behaves correctly.
See demo https://stackblitz.com/edit/angular-keyvalue-differ?file=src%2Fapp%2Fapp.component.ts

So I'm preparing a fix now and will release soon.
Thanks again for the reported bug with reproduction demo!

@Jrubzjeknf
Copy link
Author

Thanks for your quick investigation and solution! Awesome to see that this package is so well supported. 😊

Will the change also be pushed to the @no-barrels tag? Apparently I need the package from that tag to build with Ivy.

@gund gund closed this as completed in #404 Oct 14, 2020
gund pushed a commit that referenced this issue Oct 14, 2020
## [7.0.2](v7.0.1...v7.0.2) (2020-10-14)

### Bug Fixes

* **io:** only add changed inputs to changes in OnChanges hook ([1d8c6c0](1d8c6c0)), closes [#403](#403)
@gund
Copy link
Owner

gund commented Oct 14, 2020

🎉 This issue has been resolved in version 7.0.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@gund gund added the released label Oct 14, 2020
@gund
Copy link
Owner

gund commented Oct 14, 2020

Also it was released on @no-barrels tag!

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

Successfully merging a pull request may close this issue.

2 participants