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
feat(document): add $clone method #12549
Conversation
Well I dont know if we want this feature. So we need feedback from @vkarpov15 But I know for sure, that we have to keep in mind that Object.assign does only a shallow Copy. So what happens with deep nested objects? We need tests for that case. |
test/document.test.js
Outdated
|
||
assert.deepEqual(doc, clonedDoc); | ||
assert.deepEqual(doc._doc, clonedDoc._doc); | ||
assert.deepEqual(doc.$__, clonedDoc.$__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to add some extra test cases to make sure this $clone()
method works. For example:
- Modify a field in
clonedDoc
. Make sure that the field is modified inclonedDoc
, but not indoc
. - Save
clonedDoc
. Make sure that doesn't affect modified paths ondoc
. - Add a value that fails validation to
doc
. Runawait doc.validate()
. Then create a new cloned docclonedDoc = doc.$clone()
, fix the invalid value, runawait clonedDoc.validate()
. Make sure the path is still invalid inclonedDoc
. - Set a session on
clonedDoc
, make sure that doesn't affect the original doc.
lib/document.js
Outdated
clonedDoc._doc = clone(this._doc); | ||
} | ||
if (this.$__) { | ||
clonedDoc.$__ = Object.assign(Object.create(Object.getPrototypeOf(this.$__)), this.$__); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to do a deep clone on $__
, that may contain deeply nested properties.
lib/document.js
Outdated
*/ | ||
|
||
Document.prototype.$clone = function() { | ||
const clonedDoc = Object.assign(Object.create(Object.getPrototypeOf(this)), this); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't like using Object.create()
like this, because Object.create()
skips running the Document
constructor. I'd try just using const Model = this.constructor; const clonedDoc = new Model();
9de86f0
to
c7f051e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks 👍 I'll merge this into 6.8
closes #11849
Not sure if this is the best approach.
Anyway, open to feedback.