#27 Handle copying referenced objects

[darkowic]

Solves #27

Regarding using WeakMap - can we require to use a polyfill for that or we want to implement some simple implementation of it? It's widely supported across different browsers - https://caniuse.com/#search=weakmap

Benchmark - it's slightly slower with this fix. Maybe we can improve something? Or is simply WeakMap that much slower than the WeakSet?

Starting benchmarks for group simple object...
Master:
Benchmark completed for fast-copy: 2,258,798 ops/sec
Benchmark completed for fast-copy (strict): 514,319 ops/sec
After:
Benchmark completed for fast-copy: 2,235,410 ops/sec
Benchmark completed for fast-copy (strict): 479,978 ops/sec

Starting benchmarks for group complex object...
Master:
Benchmark completed for fast-copy: 100,285 ops/sec
Benchmark completed for fast-copy (strict): 34,349 ops/sec
After:
Benchmark completed for fast-copy: 82,292 ops/sec
Benchmark completed for fast-copy (strict): 30,649 ops/sec

Starting benchmarks for group circular object...
Master:
Benchmark completed for fast-copy: 1,200,336 ops/sec
Benchmark completed for fast-copy (strict): 372,298 ops/sec
After:
Benchmark completed for fast-copy: 1,193,205 ops/sec
Benchmark completed for fast-copy (strict): 354,410 ops/sec

Benchmark results complete, overall averages:
Master:
┌────────────────────┬───────────┐
│ Name               │ Ops / sec │
├────────────────────┼───────────┤
│ fast-copy          │ 667.553   │
├────────────────────┼───────────┤
│ lodash.cloneDeep   │ 506.869   │
├────────────────────┼───────────┤
│ clone              │ 445.201   │
├────────────────────┼───────────┤
│ ramda              │ 344.407   │
├────────────────────┼───────────┤
│ fast-deepclone     │ 278.045   │
├────────────────────┼───────────┤
│ deepclone          │ 250.351   │
├────────────────────┼───────────┤
│ fast-copy (strict) │ 204.973   │
├────────────────────┼───────────┤
│ fast-clone         │ 162.576   │
└────────────────────┴───────────┘
After:
┌────────────────────┬───────────┐
│ Name               │ Ops / sec │
├────────────────────┼───────────┤
│ fast-copy          │ 718.736   │
├────────────────────┼───────────┤
│ lodash.cloneDeep   │ 520.245   │
├────────────────────┼───────────┤
│ clone              │ 435.118   │
├────────────────────┼───────────┤
│ ramda              │ 390.288   │
├────────────────────┼───────────┤
│ fast-deepclone     │ 325.057   │
├────────────────────┼───────────┤
│ deepclone          │ 262.127   │
├────────────────────┼───────────┤
│ fast-clone         │ 172.48    │
├────────────────────┼───────────┤
│ fast-copy (strict) │ 155.235   │
└────────────────────┴───────────┘

[darkowic]

This benchmark is not reliable at all... I get different results all the time - but constantly the old version is slightly faster

[planttheidea]

can we require to use a polyfill for that or we want to implement some simple implementation of it? It's widely supported across different browsers - https://caniuse.com/#search=weakmap

Suddenly requiring this would be a breaking change, and I would like to avoid this as much as possible for what really is a bug fix. There is already an existing fallback implementation, and tweaking it for the same results would be trivial.

This benchmark is not reliable at all

Welcome to JavaScript benchmarks. 😋 The goal is to see themes, not treat the results as biblical (it varies from machine to machine, and even on the same machine it varies from run to run due to system load and power management).

it's slightly slower with this fix. Maybe we can improve something? Or is simply WeakMap that much slower than the WeakSet?

Slower here does not mean bad, because better accuracy is more important, especially since the reduction is small. A map will generally be a hair slower than a simple list, because the map has the relationship between two values whereas the list is just "does it exist or not", but that difference does not counteract the lack of accuracy.

I'm on vacation right now, but once I'm back I can investigate more thoroughly. Looks solid at first glance, though, once the fallback is in place.

[darkowic]

Thank you for the answer @planttheidea. I have added a simple WeakMap polyfill implementation. Should be enough as a simple fallback.

About the benchmark: I totally agree - it is slightly slower but still keeps the first place and solves the issue correctly.

[planttheidea]

LGTM!