✨ Add `constructorHandlers` option #40

alecgibson · 2024-06-12T07:13:55Z

The motivation of this change is to allow passing custom handlers for particular classes. For example, ObjectId.

These can be passed using the new constructorHandlers option:

const clone = rfdc({
  constructorHandlers: [
    [ObjectId, (o) => new ObjectId(o)],
  ],
})

Similarly, RegExp support can be added manually:

const clone = rfdc({
  constructorHandlers: [
    [RegExp, (o) => new RegExp(o)],
  ],
})

Internally, the special handlers for Date, Map, and Set are moved to use this mechanism to keep code tidy.

Limitations

Note that - for performance - this is backed under the hood by a Map with the classes as keys, which gives constant-time lookup (compared to eg iterating over an array of handlers). A limitation that this introduces is that subclasses would not be matched, and would need their own handlers, since we don't look up the prototype chain.

Performance

Benchmarks before:

benchRfdc*100: 206.839ms
benchRfdcProto*100: 206.776ms
benchRfdcCircles*100: 231.711ms
benchRfdcCirclesProto*100: 229.874ms

Benchmarks after:

benchRfdc*100: 221.126ms
benchRfdcProto*100: 239.467ms
benchRfdcCircles*100: 241.456ms
benchRfdcCirclesProto*100: 257.926ms

mcollina · 2024-06-12T08:14:45Z

Can you document this in the README?

mcollina · 2024-06-12T08:19:47Z

index.js

-        o2[k] = new Map(cloneArray(Array.from(cur), clone))
-      } else if (cur instanceof Set) {
-        o2[k] = new Set(cloneArray(Array.from(cur), clone))
+      } else if (constructorHandlers.has(cur.constructor)) {


Suggested change

} else if (constructorHandlers.has(cur.constructor)) {

} else if (cur.constructor !== Object && constructorHandlers.has(cur.constructor)) {

I think we add a fast path wherever there is a .has() to bypass the map lookup.

This should allow to avoid the perf regression.

Have pushed the change. It improves performance, but not back to where it was before:

benchRfdc*100: 212.82ms benchRfdcProto*100: 229.3ms benchRfdcCircles*100: 255.588ms benchRfdcCirclesProto*100: 238.306ms

Okay I've assigned in the if check to avoid calling both .has() and .get(), and benchmarks are improved to:

benchRfdc*100: 203.999ms benchRfdcProto*100: 215.779ms benchRfdcCircles*100: 224.12ms benchRfdcCirclesProto*100: 243.172ms

Which is vaguely in-line with before.

mcollina

lgtm

mcollina · 2024-06-12T09:06:43Z

Can you rebase your PR? I've fixed CI.

The motivation of this change is to allow passing custom handlers for particular classes. For example, [`ObjectId`][1]. These can be passed using the new `constructorHandlers` option: ```js const clone = rfdc({ constructorHandlers: [ [ObjectId, (o) => new ObjectId(o)], ], }) ``` Similarly, `RegExp` support can be added manually: ```js const clone = rfdc({ constructorHandlers: [ [RegExp, (o) => new RegExp(o)], ], }) ``` Internally, the special handlers for `Date`, `Map`, and `Set` are moved to use this mechanism to keep code tidy. Limitations ----------- Note that - for performance - this is backed under the hood by a `Map` with the classes as keys, which gives constant-time lookup (compared to eg iterating over an array of handlers). A limitation that this introduces is that subclasses would not be matched, and would need their own handlers, since we don't look up the prototype chain. Performance ----------- Benchmarks before: ``` benchRfdc*100: 206.839ms benchRfdcProto*100: 206.776ms benchRfdcCircles*100: 231.711ms benchRfdcCirclesProto*100: 229.874ms ``` Benchmarks after: ``` benchRfdc*100: 221.126ms benchRfdcProto*100: 239.467ms benchRfdcCircles*100: 241.456ms benchRfdcCirclesProto*100: 257.926ms ``` [1]: davidmarkclements#7

Improves benchmarks to: ``` benchRfdc*100: 212.82ms benchRfdcProto*100: 229.3ms benchRfdcCircles*100: 255.588ms benchRfdcCirclesProto*100: 238.306ms ```

Improves benchmarks to: ``` benchRfdc*100: 203.999ms benchRfdcProto*100: 215.779ms benchRfdcCircles*100: 224.12ms benchRfdcCirclesProto*100: 243.172ms ```

``` standard: Use JavaScript Standard Style (https://standardjs.com) /home/runner/work/rfdc/rfdc/test/index.js:124:25: Use a regular expression literal instead of the 'RegExp' constructor. (prefer-regex-literals) ```

BridgeAR

This is a nice addition!

BridgeAR · 2024-06-12T09:15:44Z

index.js

+  const constructorHandlers = new Map([
+    [Date, (o) => new Date(o)],
+    [Map, (o, fn) => new Map(cloneArray(Array.from(o), fn))],
+    [Set, (o, fn) => new Set(cloneArray(Array.from(o), fn))]
+  ].concat(opts.constructorHandlers || []))


Suggested change

const constructorHandlers = new Map([

[Date, (o) => new Date(o)],

[Map, (o, fn) => new Map(cloneArray(Array.from(o), fn))],

[Set, (o, fn) => new Set(cloneArray(Array.from(o), fn))]

].concat(opts.constructorHandlers || []))

const constructorHandlers = new Map()

constructorHandlers.add(Date, (o) => new Date(o))

constructorHandlers.add(Map, (o, fn) => new Map(cloneArray(Array.from(o), fn)))

constructorHandlers.add(Set, (o, fn) => new Set(cloneArray(Array.from(o), fn)))

if (opts.constructorHandlers) {

opts.constructorHandlers.forEach((name, handler) => constructorHandlers.add(name, handler)

}

It is faster to work around the additional array allocation. That could also be done inside of the Map and Set handlers itself.

I don't see much difference in the benchmark, but will make the change anyway

benchRfdc*100: 206.686ms benchRfdcProto*100: 218.722ms benchRfdcCircles*100: 228.578ms benchRfdcCirclesProto*100: 239.244ms

Updated benchmark ``` benchRfdc*100: 206.686ms benchRfdcProto*100: 218.722ms benchRfdcCircles*100: 228.578ms benchRfdcCirclesProto*100: 239.244ms ```

Adds TypeScript definitions for davidmarkclements#40

Adds TypeScript definitions for #40

mcollina reviewed Jun 12, 2024

View reviewed changes

mcollina approved these changes Jun 12, 2024

View reviewed changes

alecgibson added 4 commits June 12, 2024 10:07

📝 Add constructorHandlers option to readme

68a7ef5

⚡️ Add constructor handler fast path

63b4d45

Improves benchmarks to: ``` benchRfdc*100: 212.82ms benchRfdcProto*100: 229.3ms benchRfdcCircles*100: 255.588ms benchRfdcCirclesProto*100: 238.306ms ```

⚡️ Assign handler in if statement

f08c7ec

Improves benchmarks to: ``` benchRfdc*100: 203.999ms benchRfdcProto*100: 215.779ms benchRfdcCircles*100: 224.12ms benchRfdcCirclesProto*100: 243.172ms ```

alecgibson force-pushed the constructor-lookup branch from f3b9655 to f08c7ec Compare June 12, 2024 09:08

🚨 Fix linter warning about RegExp

075a0cc

``` standard: Use JavaScript Standard Style (https://standardjs.com) /home/runner/work/rfdc/rfdc/test/index.js:124:25: Use a regular expression literal instead of the 'RegExp' constructor. (prefer-regex-literals) ```

BridgeAR reviewed Jun 12, 2024

View reviewed changes

⚡️ Avoid extra array allocation

b4c0af0

Updated benchmark ``` benchRfdc*100: 206.686ms benchRfdcProto*100: 218.722ms benchRfdcCircles*100: 228.578ms benchRfdcCirclesProto*100: 239.244ms ```

mcollina merged commit 8c6bfec into davidmarkclements:master Jun 12, 2024
8 checks passed

alecgibson deleted the constructor-lookup branch June 12, 2024 09:48

alecgibson added a commit to alecgibson/rfdc that referenced this pull request Jun 12, 2024

🏷️ Add type definitions for constructorHandlers option

7991bbb

Adds TypeScript definitions for davidmarkclements#40

alecgibson mentioned this pull request Jun 12, 2024

🏷️ Add type definitions for constructorHandlers option #43

Merged

mcollina pushed a commit that referenced this pull request Jun 12, 2024

🏷️ Add type definitions for constructorHandlers option (#43)

6330dc0

Adds TypeScript definitions for #40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add `constructorHandlers` option #40

✨ Add `constructorHandlers` option #40

alecgibson commented Jun 12, 2024

mcollina commented Jun 12, 2024

mcollina Jun 12, 2024

alecgibson Jun 12, 2024

alecgibson Jun 12, 2024

mcollina left a comment

mcollina commented Jun 12, 2024

BridgeAR left a comment

BridgeAR Jun 12, 2024

alecgibson Jun 12, 2024

	} else if (constructorHandlers.has(cur.constructor)) {
	} else if (cur.constructor !== Object && constructorHandlers.has(cur.constructor)) {

-  const constructorHandlers = new Map([
-    [Date, (o) => new Date(o)],
-    [Map, (o, fn) => new Map(cloneArray(Array.from(o), fn))],
-    [Set, (o, fn) => new Set(cloneArray(Array.from(o), fn))]
-  ].concat(opts.constructorHandlers || []))
+  const constructorHandlers = new Map()
+  constructorHandlers.add(Date, (o) => new Date(o))
+  constructorHandlers.add(Map, (o, fn) => new Map(cloneArray(Array.from(o), fn)))
+  constructorHandlers.add(Set, (o, fn) => new Set(cloneArray(Array.from(o), fn)))
+  if (opts.constructorHandlers) {
+    opts.constructorHandlers.forEach((name, handler) => constructorHandlers.add(name, handler)
+  }

✨ Add constructorHandlers option #40

✨ Add constructorHandlers option #40

Conversation

alecgibson commented Jun 12, 2024

Limitations

Performance

mcollina commented Jun 12, 2024

mcollina Jun 12, 2024

Choose a reason for hiding this comment

alecgibson Jun 12, 2024

Choose a reason for hiding this comment

alecgibson Jun 12, 2024

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Jun 12, 2024

BridgeAR left a comment

Choose a reason for hiding this comment

BridgeAR Jun 12, 2024

Choose a reason for hiding this comment

alecgibson Jun 12, 2024

Choose a reason for hiding this comment

✨ Add `constructorHandlers` option #40

✨ Add `constructorHandlers` option #40