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

[RFC] Collections in changesets #1329

Closed
malarzm opened this issue Dec 31, 2015 · 2 comments
Closed

[RFC] Collections in changesets #1329

malarzm opened this issue Dec 31, 2015 · 2 comments
Labels
Milestone

Comments

@malarzm
Copy link
Member

malarzm commented Dec 31, 2015

Currently collections are not always included in the changeset. What would you like to see there? (As a reminder: changeset for a field is a tuple of [oldValue, newValue])

  1. [original collection instance, current collection instance]
  2. [snapshot, current data]
  3. [hydrated data from db's collection, current data]
  4. [removed elements, added elements]

My point of view:

  1. doesn't make much sense as collections SHOULD be the same all the time BUT it gives most power and allows us to not think about initialization/any other checks cascading this responsibility to developer. Also in contrast to 4. it will allow to detect position changes if one would need them. Also we can add getAddedDocuments which will work like getDeletedDocuments effectively allowing 4th idea (for which I'd add exactly same method)
  2. easy but requires initialization which may be unwanted, can break if collection's instance was changed
  3. similar to 2, just will remember original snapshot in case ->clear() was called and won't break when collection changed, still will always initialize collection since current data is needed
  4. somehow I like it most but it can easily break if collection instance was changed

Having written this down I'm leaning towards 1, looking for thoughts :)

@malarzm malarzm added the Idea label Dec 31, 2015
@alcaeus
Copy link
Member

alcaeus commented Dec 31, 2015

Here's my point of view:

  1. In my opinion it's the only thing that adheres to the [oldValue, newValue] tuple. The thing is, checking whether the value has changed is difficult since you're getting the same objects in most cases and need additional logic.
  2. Initialization just to provide a snapshot may be unwanted, especially since it may not be necessary, e.g. appending to a collection can be done without a changeset.
  3. This re-introduces a problem when clearing large collections: clearing an uninitialized collection will not trigger an initialize to avoid hydrating a bunch of documents that don't need to be hydrated anyways (although clear does initialize collections with orphanRemoval enabled but that's the exception rather than the norm)
  4. This really goes away from the [oldValue, newValue] tuple but it would bring the data most developers are looking for: actual changes. Remember though, this would also require initializing a collection that was cleared in order to provide the elements that were removed.

Right now I'm tending to option 1 and leaving it up to developers to handle collections in changesets should they want to act on it. Since collections are not present in the changeset now we also shouldn't be breaking functionality with this.

@malarzm
Copy link
Member Author

malarzm commented Jan 16, 2016

#1339 was merged :)

@malarzm malarzm closed this as completed Jan 16, 2016
@malarzm malarzm added this to the 1.1 milestone Jan 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants