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

feat(clone) Check for & use cloneOf method when cloning objects #8307

Closed
wants to merge 1 commit into from

Conversation

stieg
Copy link
Contributor

@stieg stieg commented Nov 6, 2019

Adds a check in the clone function to see if the given parameter has
a cloneOf method. If the method exists then the clone method
assumes that this method will return a clone of the called object and
thus invokes it to complete its task.

Fixes #8299

Adds a check in the `clone` function to see if the given parameter has
a `cloneOf` method.  If the method exists then the `clone` method
assumes that this method will return a clone of the called object and
thus invokes it to complete its task.

Fixes Automattic#8299
@stieg
Copy link
Contributor Author

stieg commented Nov 7, 2019

@vkarpov15 The failure of the CI appears to be unrelated to this change. Looking for feedback on this when you have a moment. This PR currently blocks any resolution to mongoosejs/mongoose-double#7 so as soon as a path forward is cleared here I can move forward with PRs for that project. Cheers.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

I don't think I can merge this, especially in a patch release. cloneOf() simply isn't a well established enough concept for me to justify putting it this deep in the codebase. Too many potential surprises if people pass in objects that define a cloneOf() function.

I haven't had a chance to look into #8299 enough yet to see whether it is viable for us to handle cloning doubles in a way that doesn't involve adding another property to the double type. But one alternative would be for Mongoose to export a clone symbol that mongoose-double could then use to expose a function that Mongoose could use to clone doubles, so we wouldn't conflict with user-defined cloneOf() functions.

@stieg
Copy link
Contributor Author

stieg commented Nov 8, 2019

I agree with your assessment above and the potential proposed solution. Question I have is if I put something like that together are you ready to review/accept it or do you simply need more time to review #8299 before even considering it. I would like to drive this issue to a conclusion soon as is reasonably possible but first I need your blessing on the solution path before I can do so. LMK.

@vkarpov15 vkarpov15 closed this Nov 8, 2019
@Automattic Automattic locked as resolved and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add cloneOf or clone method check in clone function to handle odd cloning situations
2 participants