Closed chrisands closed 7 months ago
Yeah feel free to make a PR, seems like a cheap check for added safety. I'm assuming something like this?
if (!prototype || !prototype.constructor) {
return create(null);
}
EDIT: Actually, now that I've gotten fingers to keyboard, I think this can be solved in a more accurate way that also doesn't impact perf for the majority use-case.
When cloning an object, this utility is used:
export function getCleanClone(prototype: any): any {
if (!prototype) {
return create(null);
}
const Constructor = prototype.constructor;
if (Constructor === Object) {
return prototype === Object.prototype ? {} : create(prototype);
}
if (~toStringFunction.call(Constructor).indexOf('[native code]')) {
try {
return new Constructor();
} catch {}
}
return create(prototype);
}
The failure you're getting is in the final if
condition, which is for custom classes. Empty
definitely falls into this category, so we can just add an existy check prior to getting the index of the constructor:
if (
Constructor &&
~toStringFunction.call(Constructor).indexOf('[native code]')
) {
Now the test passes, and the clone appropriately has the same prototype as the Empty
instance. Expect a PR soon.
Version 3.0.2
has been released with this fix. If you have any more issues, let me know!
Here from pinojs/pino-pretty#477
Basically I get an error from trying to log null object that uses V8 optimization
Optimization looks like this and it used in
fast-querystring
packageI figured out that internal function
getCleanClone
only check that prototype existI think it should handle case where
var Constructor = prototype.constructor === undefined
that would mean that this is null prototype and we should returncreate(null)
. I could make PR if you agree with it. What do you think?Also it possible to use this optimization, but I'm not sure how much it would be breaking change and should be perf tested to understand if this is worth it. Separate issue ofc.