Repository navigation
Add Doc.toSnapshot - #508
Conversation
|
CI is failing for what looks like a silly reason unrelated to the changes here.
Any ideas on how to turn CI green? |
Co-authored-by: Alec Gibson <[email protected]>
…doc-snapshot-method
|
What is the suggested approach for doing the cloning? I see there are two existing implementations of Line 176 in 3e636d0 function clone(obj) {
return (obj === undefined) ? undefined : JSON.parse(JSON.stringify(obj));
}This is not exported. The same implementation is found in https://github.com/share/sharedb/blob/master/test/projections.js#L389 My instinct is to refactor Alternatively, I could copy-paste the implementation into Doc.js. That would make 3 copies of the same implementation. Thoughts? |
|
CI is failing due to something seemingly unrelated to these changes.
https://github.com/share/sharedb/pull/508/checks?check_run_id=3475145150 |
|
Actually, I'm not sold on the |
|
Notes from call:
|
|
Requesting re-review @alecgibson @ericyhwang . Thanks! CI is failing on ShareDB Mongo, not sure why (and not sure why this would not be the case in |
| // to guard against mutations. You can opt out of cloning by passing `true` | ||
| // as the optional `doNotClone` argument. | ||
| // | ||
| // @param doNotClone (optional) If `true`, the document data is not cloned. This can be used |
There was a problem hiding this comment.
I'm not sure we should let people do this. I know cloning is a performance overhead, but the failure mode is bad if consumers accidentally mutate the snapshot (because they'll mutate doc.data, which shouldn't happen outside of OT).
There was a problem hiding this comment.
The main use case for this is to serialize the snapshot and send it over the network. I'm of the opinion that consumers should be allowed to opt out of cloning if they know for sure that their code will never mutate the data.
There was a problem hiding this comment.
I still personally think that the risks of weird side-effects massively outweigh the relatively small performance impact of cloning. Cloning is a perfectly normal, defensive thing to do. json0 itself uses clone in many places through its lifecycle.
(As a side-note, json0 is just using a JSON approach to cloning, although I believe the comment about it being fast is out-of-date).
There was a problem hiding this comment.
@curran I discussed with @ericyhwang and we think we shouldn't include this option for now. Let's keep the API footprint small and fool-proof where possible.
If you find that this does impact performance with some benchmarks, then we can consider adding an override later.
There was a problem hiding this comment.
Got it. That simplifies things. Will do!
| exports.clone = function(obj) { | ||
| return (obj === undefined) ? undefined : JSON.parse(JSON.stringify(obj)); | ||
| }; |
There was a problem hiding this comment.
This code has only hitherto been used in tests, and I wonder if we should use something more performant in production code?
If we're okay with adding a dependency, we've been using rfdc and it's worked well.
On the other hand, the performance gain is (relatively) small (~3x speed-up), so I don't mind just sticking with this.
@ericyhwang thoughts?
There was a problem hiding this comment.
This code has only hitherto been used in tests
It was also used in lib/db/memory.js.
There was a problem hiding this comment.
The MemoryDB shouldn't be used in production, so this clone method still hasn't touched production code before now.
|
@curran we've dropped Node.js v10 support now. You should be able to rebase and greenify the build. |
|
Addressed feedback and merged latest from |
|
Hooray! Thank you. |
4 participants
Fix #500