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 ManyArray should handle duplicates gracefully #9126

Merged
merged 47 commits into from
Dec 4, 2023

Conversation

runspired
Copy link
Contributor

@runspired runspired commented Nov 18, 2023

resolves #9119

New Tests

Graph Tests (we will likely want these in 5.3 version of this, but not necessarily)

  • [-] addRelatedRecords mutation dedupes
  • [-] addRelatedRecords mutation asserts duplicates if we want it to
  • replaceRelatedRecords mutation dedupes
  • [-] replaceRelatedRecords mutation can assert duplicates if we want it to
  • [-] test local graph mutation notification behaviors for addRelatedRecords, removeRelatedRecords, replaceRelatedRecords

Model/Main Tests

  • ManyArray mutation dedupes for replace
  • ManyArray mutation dedupes for splice in with adds
  • ManyArray mutation dedupes for splice in with adds & removes
  • ManyArray mutation dedupes for push
  • ManyArray mutation dedupes for unshift
  • ManyArray is not overly notified/churning on mutation
  • ManyArray can optionally assert for duplicates

@github-actions github-actions bot added the backport-lts PR targets the current lts branch label Nov 18, 2023
@runspired runspired added the 🏷️ bug This PR primarily fixes a reported issue label Nov 18, 2023
self[MUTATE]!(prop as string, args, result);
addToTransaction(_TAG);
// TODO handle cache updates
const result = self[MUTATE]!(target, receiver, prop as string, args, _TAG);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thing I've been constantly debating around this line is whether we should try/finally it to ensure transaction is set back to false if we error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-lts PR targets the current lts branch 🏷️ bug This PR primarily fixes a reported issue
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants