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

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

Closed
chrisands opened this issue Nov 1, 2023 · 2 comments · Fixed by #507
Closed

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

chrisands opened this issue Nov 1, 2023 · 2 comments · Fixed by #507

Comments

@chrisands
Copy link
Contributor

chrisands commented Nov 1, 2023

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"
@jsumners
Copy link
Member

jsumners commented Nov 1, 2023

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

@chrisands
Copy link
Contributor Author

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)

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

Successfully merging a pull request may close this issue.

2 participants