Fix deepClone property accessor for an special case

[Hugome]

In add to the previous commit :
e1f8da4
To cover special case when the key set/get exist but == undefined :

[apollo-cla]

@Hugome: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[benjamn]

Perhaps we should just unconditionally delete the properties? I know delete is sometimes considered slower than assigning undefined or null (though of course the semantics are different), but deleting a non-existent property should be at least as fast as checking for the property first and then deleting it.

[Hugome]

Your are right, plus deleting non existent property doesn't throw any kind of error/exception.
Push done

[beeplin]

@benhjames @Hugome Unfortunately I tested and found this PR would severely slow down the JS running in browser. Without delete desc.get/set it will cause the JS error previously mentioned, and with delete, after some times of mutations it will be slower and slower, until Chrome warns that it will be out of memory.

As far as I see the 2.4.2 works fine with the old deepClone. Is it necessary to change it to the new one?

[beeplin]

If it is not an option to switch back to the old deepClone, I propose to use this:

          if (Object.hasOwnProperty.call(desc, 'get')) delete desc.get
          if (Object.hasOwnProperty.call(desc, 'set')) delete desc.set

Somehow I still feel this is a little bit slower than the old implementation of deep clone, but at least it will handle the set: undefined edge case and is much faster than direct deleting without checking.

[benjamn]

I have trouble believing that a delete of a missing property—which should essentially be a no-op—would cause severe memory problems in Chrome. The delete operator has been a feature of JavaScript since before Chrome or V8 existed. Please provide evidence to back up your claims.

[beeplin]

I did more tests and now I guess it is not the delete but some other part of the new cloneDeep.js that's causing the slowing problem. The Object.hasOwnProperty way I mentioned above is actually as slow as delete directly.

Sorry for that misleading. Will dig more.

And again, in fact the old implementation works fine.

[benjamn]

One thing we can do is not worry about symbol properties or non-enumerable keys, since the previous fclone implementation just used Object.keys. That would simplify the implementation, but still retain the handling of cycles.

[benjamn]

@beeplin Check out PR #4052!

[beeplin]

yeah @benjamn #4052 works and has no performance problem. 👍