davidmarkclements / rfdc

Really Fast Deep Clone
MIT License
635 stars 24 forks source link

Should `proto` default to `true`? #11

Closed silverwind closed 5 years ago

silverwind commented 5 years ago

I see no downside to having it set, it's faster and it does a more accurate clone. Or is there?

BridgeAR commented 5 years ago

I personally prefer to have an explicit opt-in behavior. That way a user is explicitly forced to set that option in case they want the behavior and they know what it means.

It could otherwise result in some unexpected behavior in rare cases: most people expect a deep clone to work similar to JSON.parse(JSON.stringify(value)). Using that to clone an object ignores the prototype and removes all "magical" parts from their objects and that could be used to pass these objects around and to pass it through to other APIs. Or in other words: some times a deep clone might be used to strip special things from an internal structure. It is probably more difficult to notice that the prototype was cloned but it should not have been than noticing that the prototype was not cloned but it should have been. And I prefer explicit behavior in this case.

silverwind commented 5 years ago

I tend to agree, clone should probably be dumb by default. For my case, the prototype-less copy is sufficient. I was just wondering why the fastest method was not the default, or rather why it's faster to copy with prototypes than without, but that's probably a implementation detail of v8.

BridgeAR commented 5 years ago

why it's faster to copy with prototypes than without

The reason is that there's one more check:

I used a for ... in loop instead of e.g. for (const prop of Object.keys(obj)) {}. So we have to filter the prototype properties:https://github.com/davidmarkclements/rfdc/blob/a9fa9599a86ad7949beaf740c150138614d1f5ae/index.js#L101

silverwind commented 5 years ago

Ah, I see. Might eventually replace that with for (var k of Object.keys(o)) once that is sufficiently optimized in v8, but it's currently around 30% slower than filtered for-in.