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

Optional delta style object for update operations in changes system #578

Closed
ChristopherTotty opened this issue Jun 26, 2017 · 5 comments
Closed
Labels

Comments

@ChristopherTotty
Copy link

Currently when using the changes system an update is stored as a complete copy of the object. It would be beneficial in some cases to instead store a delta object with only the properties that have been changed from the original.

This would be most helpful when using LokiJS as a persistence layer for client data that will be synced to a central database on a server, since it would reduce the amount of traffic that is sent as part of that sync process.

I am currently working on a project where this is the case, and I am considering making this change for my own use.

Would there be any interest in a pull request for this feature, and if so, is there any opinion on it before I get started?

@obeliskos
Copy link
Collaborator

Sure, that would be great 👍

By default, collections do not have cloning enabled so all returned objects are the same instances as those used internally. If I do a find and update the object(s), the internal references are already updated before I call the update() method which essentially just rebuilds indices and emits events.

So if cloning is not enabled, I guess you would have to 'pre-emptively' add cloned baseline copies of all documents when initializing changes api on collections that opt in. Then I guess you would not include those in changes, but upon generating changes you would need to rebaseline 'non-dirty' baseline objects. With those copies, you can compare each updated object with highest (most recent) history object (or if empty) the baseline object. Does that make sense?

@techfort wrote changes api so if he can think of any issues maybe he can chime in.

@ChristopherTotty
Copy link
Author

Ah, I didn't think about cloning since we added our own implementation that is run on everything as it is pulled out of the database.

Seems like doubling the memory consumption of every collection that opts in would be a turn off.

What would you think about either requiring cloning to be enabled or potentially building that clone set as items are selected from the collection? That way memory use would start at the same level as the current implementation and slowly grow to potential max of double as items are pulled from the collection.

@obeliskos
Copy link
Collaborator

So let's say you add an collection constructor option... something like 'deltaChangesApi' which you can turn on. If they turn that on I can see us forcing 'clones' collection option to be 'true' whether they pass it or not... possibly even override cloneMethod to ensure we use 'parse-stringify' instead of 'shallow' cloning methods which may not be effective at tracking changes to nested objects.

Since you are introducing a new option, as long as the old behavior continues to work as it did before, you can override the other options as it makes sense to do so. Does that answer your concerns?

With cloning enabled there is still (at least temporary) memory overhead and performance hit to clone internal objects as they come in and go out through find/insert/update/etc ops. I can see that being perfectly fine though as your feature would be concerned with reducing transfer size of changesets.

@ChristopherTotty
Copy link
Author

Sounds good, a constructor option and forcing cloning should work well. I hope to look at this in the next few days.

ChristopherTotty pushed a commit to ChristopherTotty/LokiJS that referenced this issue Jul 7, 2017
…tion output to modified properties only. Also includes properties flagged as unique, since they would be needed to identify the item that is updated.
obeliskos added a commit that referenced this issue Jul 19, 2017
#578: Add optional flag for Changes API to limit update operation out…
Viatorus added a commit to LokiJS-Forge/LokiDB that referenced this issue Oct 5, 2017
…on output to modified properties only (#29)

Also includes properties flagged as unique, since they would be needed to identify the item that is updated. (from techfort/LokiJS#578)
Viatorus added a commit to LokiJS-Forge/LokiDB that referenced this issue Nov 22, 2017
…nsform

Using new 'CloneMethod.SHALLOW_RECURSE_OBJECTS' for transform parameter substitution to avoid error when base transform step(s) have non serializable properties. (from techfort/LokiJS#578)
Viatorus added a commit to LokiJS-Forge/LokiDB that referenced this issue Nov 22, 2017
…ansform (#43)

Using new 'CloneMethod.SHALLOW_RECURSE_OBJECTS' for transform parameter substitution to avoid error when base transform step(s) have non serializable properties. (from techfort/LokiJS#578)
@stale
Copy link

stale bot commented Jul 13, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

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