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

Fix mutating array datasource bug #1109

Conversation

KashishGoel
Copy link

@KashishGoel KashishGoel commented Feb 22, 2018

Changes in this pull request

Copy objects when retrieving from datasource to account for the edge case where the returned data is a mutable array.

Issue fixed: #999

Checklist

  • All tests pass. Demo project builds and runs.
  • I added tests, an experiment, or detailed why my change isn't tested.
  • I added an entry to the CHANGELOG.md for any breaking changes, enhancements, or bug fixes.
  • I have reviewed the contributing guide

Copy link
Contributor

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Thanks @KashishGoel ! 👍

Copy link
Contributor

@jessesquires jessesquires left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh actually, please add a changelog entry for this for 3.3.0 😊

@rnystrom
Copy link
Contributor

@KashishGoel responding to the Test question here. Actually think a Test would be awesome for the longevity of this fix! Would be really easy:

  • Create a mutable array
  • Return array in binding SC method
  • Mutate array
  • Assert view models isn’t mutated

You can init a new SC without setting up the rest of the IGLK infra even.

Sent with GitHawk

@facebook-github-bot
Copy link
Contributor

@KashishGoel has updated the pull request. View: changes

@facebook-github-bot
Copy link
Contributor

@KashishGoel has updated the pull request. View: changes

@KashishGoel
Copy link
Author

@rnystrom Added a test, should be enough to ensure we don't run into this problem again.

@rnystrom
Copy link
Contributor

@KashishGoel its perfect 😇

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rnystrom is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

IGListBindingSectionController updateAnimated have copy BUG
4 participants