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

Add BSON ObjectId support #7

Closed
niftylettuce opened this issue Jun 17, 2019 · 3 comments
Closed

Add BSON ObjectId support #7

niftylettuce opened this issue Jun 17, 2019 · 3 comments

Comments

@niftylettuce
Copy link

Right now, even with { proto: true }, ObjectId's are cloned inaccurately, resulting in errors like this when one attempts to even console.log them after they have been cloned:

✖  error     TypeError: argument should be a Buffer
    at stringSlice (buffer.js:586:20)
    at Object.Buffer.toString (buffer.js:633:10)
    at Object.inspect (buffer.js:652:14)
    at formatValue (util.js:430:38)
    at formatProperty (util.js:831:11)
    at formatObject (util.js:647:17)
    at formatValue (util.js:609:18)
    at formatProperty (util.js:831:11)
    at formatObject (util.js:647:17)
    at formatValue (util.js:609:18)
    at formatProperty (util.js:831:11)
    at formatArray (util.js:729:17)
    at formatValue (util.js:609:18)
    at inspect (util.js:324:10)
    at format (util.js:253:18)
    at Console.log (console.js:130:21)
@niftylettuce
Copy link
Author

@BridgeAR
Copy link
Collaborator

This would require a new option. The proto option allows to copy prototype properties, not the prototype itself. It was meant as a performance improvement (that is mostly not required with the latest implementation).

Adding an option like that might be a good idea but it would likely be significantly slower. I can also imagine that I currently miss some edge cases that could not fully be handled even when cloning the prototype (we should probably also copy properties with the correct property descriptor, if we add such a mode).

alecgibson added a commit to alecgibson/rfdc that referenced this issue Jun 12, 2024
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
alecgibson added a commit to alecgibson/rfdc that referenced this issue Jun 12, 2024
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
mcollina pushed a commit that referenced this issue Jun 12, 2024
* ✨ Add `constructorHandlers` option

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]: #7

* 📝 Add `constructorHandlers` option to `readme`

* ⚡️ Add constructor handler fast path

Improves benchmarks to:

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

* ⚡️ Assign handler in `if` statement

Improves benchmarks to:

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

* 🚨 Fix linter warning about `RegExp`

```
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)
```

* ⚡️ Avoid extra array allocation

Updated benchmark

```
benchRfdc*100: 206.686ms
benchRfdcProto*100: 218.722ms
benchRfdcCircles*100: 228.578ms
benchRfdcCirclesProto*100: 239.244ms
```
@alecgibson
Copy link
Contributor

alecgibson commented Jun 12, 2024

You can now do this as of v1.4.0:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants