pinojs / pino-noir

🌲 pino log redaction 🍷
MIT License
66 stars 17 forks source link

Is the use of eval still the best approach? #19

Closed nkhil closed 3 years ago

nkhil commented 3 years ago

I noticed that eval is used in pino-noir

// we use eval to pre-compile the redactor function
  // this gives us up 100's of ms (per 10000ops) in some
  // cases (deep nesting, wildcards). This is certainly
  // safe in this case, there is not user input here.
  /* eslint no-eval: 0 */
  function factory (paths) {
    var redactor
    eval('redactor = function redactor (o) { return redact(o, ' + JSON.stringify(paths) + ') }')
    redact() // shh linter
    return redactor

After reading around a bit, I wasn't sure how eval was being used here to precompile the redactor function, and how it was being more efficient/faster?

Also asking since I read this on MDN:

eval() is also slower than the alternatives, since it has to invoke the JavaScript interpreter, while many other constructs are optimized by modern JS engines.

is there are any benchmarks on using eval vs not using it?

mcollina commented 3 years ago

We could use new Function(), would you like to send a PR?

nkhil commented 3 years ago

Thanks @mcollina , yes I'd love to create a PR! I'll come back to you shortly!

nkhil commented 3 years ago

@mcollina I tried replacing eval() with a regular function (example below) and ran a benchmark test after 50k iterations each. eval() performed ~23% better.

    const redactor = o => redact(o, JSON.stringify(paths))
    return redactor

I don't know enough about eval() or new Function() currently to make that change. Based on my own benchmarking, I'm happy to go with eval() and close this issue.

However, would you be so kind to describe how eval() is able to be faster compared to a regular function? I'm very curious.

mcollina commented 3 years ago

I recommended to use new Function() instead of eval(). It's safer and it perform the same.

nkhil commented 3 years ago

@mcollina I've played around with this some more, and I'm still unsure on how to access functions that are in scope.

For eg:

const addOne = num => num + 1;

const fn = new Function(10, '(num) => addOne(num)')

fn()

^ This throws an error.

Since I'm unsure on how to proceed to create a PR, I'm going to close this issue.