pinojs/pino-pretty

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

chrisands opened this issue · 2 comments

It seems that pino-pretty crashes when tries to serialize object with null prototype.

Ran into problem when tried to log request.query from fastify.

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/fast-copy@3.0.1/node_modules/fast-copy/dist/cjs/index.cjs:49:27)
    at copyObjectLooseModern (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/fast-copy@3.0.1/node_modules/fast-copy/dist/cjs/index.cjs:213:17)
    at copier (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/fast-copy@3.0.1/node_modules/fast-copy/dist/cjs/index.cjs:362:20)
    at copy (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/fast-copy@3.0.1/node_modules/fast-copy/dist/cjs/index.cjs:375:16)
    at filterLog (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/pino-pretty@10.2.3/node_modules/pino-pretty/lib/utils/filter-log.js:30:19)
    at Object.pretty (/Users/osiyuk/Projects/github/pino-pretty-issue/node_modules/.pnpm/pino-pretty@10.2.3/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)

Reproduction:

const pp = require('pino-pretty');
const qs = require('fast-querystring')

const pretty = pp.prettyFactory();

const nullObj = qs.parse('{}')

process.stdout.write(pretty(nullObj) + '\n');
    "fast-querystring": "^1.1.2",
    "pino-pretty": "^10.2.3"

Ugh, I absolutely despise Object.create(null). Would you like to submit a PR to fix this? Remember to add unit tests.

Just found time to tackle the issue. Here's what I found out.

Object.create(null) works fine and doesn't have problem. The problem is that fast-querystring uses V8 optimization by defining function replacing it's prototype

// 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'm not sure if it is a good idea to go tweak fast-copy to handle this case. @jsumners WDYT?

Notes about fast-copyfunction getCleanClone should handle case where var Constructor = prototype.constructor === undefined that would mean that this is null prototype and we should return create(null)