Function.prototype.toString requires that 'this' be a Function

[chrisands]

Here from pinojs/pino-pretty#477

Basically I get an error from trying to log null object that uses V8 optimization

TypeError: Function.prototype.toString requires that 'this' be a Function
    at toString (<anonymous>)
    at getCleanClone (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/fast-copy/dist/cjs/index.cjs:49:27)
    at copyObjectLooseModern (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/fast-copy/dist/cjs/index.cjs:213:17)
    at copier (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/fast-copy/dist/cjs/index.cjs:362:20)
    at copy (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/fast-copy/dist/cjs/index.cjs:375:16)
    at filterLog (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/pino-pretty/lib/utils/filter-log.js:30:19)
    at Object.pretty (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/[email protected]/node_modules/pino-pretty/lib/pretty.js:81:11)
    at Object.<anonymous> (/Users/osiyuk/Projects/github/pino-pretty-issue/index.js:18:14)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)

Optimization looks like this and it used in fast-querystring package

// Optimization: Use new Empty() instead of Object.create(null) for performance
// v8 has a better optimization for initializing functions compared to Object
const Empty = function () {};
Empty.prototype = Object.create(null);

I figured out that internal function getCleanClone only check that prototype exist

  if (!prototype) {
    return create(null);
  }

I think it should handle case where var Constructor = prototype.constructor === undefined that would mean that this is null prototype and we should return create(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.

[planttheidea]

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.

[planttheidea]

Version 3.0.2 has been released with this fix. If you have any more issues, let me know!