Contradicting content: named functions vs anonymous arrow functions about nodebestpractices HOT 21 CLOSED

A little complex but:

• Use arrow functions where the function is assigned to a variable or a property (which names it).
• Use arrow functions in very short functions (map filter reduce etc) but keep them on separate lines if they might throw.
• Name functions that are passed as callbacks (prefer promises) or event handlers so that they appear more easily in the profiler or stack traces.

from nodebestpractices.

@i0natan that is because the timeout callback (Timeout.setTimeout [as _onTimeout]) is run in a new async-context. That's one of the bigger banes of node's existence, and is why async-hooks were developed (in all their previous incarnations). There are npm modules that patch this, such as @AndreasMadsen's trace, another guy's asynctrace, and a package named after winter underwear.

Context: The asynchronicity in node is not part of the language, and so is not implemented in the VM (yet?). So every time the runtime finished a Javascript "Tick" the call stack is emptied, and when node "calls back" into JS it starts a new call stack.

BTW: setTimeout is also not part of the language but rather is spec'ed in the HTML standard

from nodebestpractices.

    at Timeout.setTimeout [as _onTimeout] (C:\code\playground\test.js:5:32)

Correct file name, line and column which lets me know where the error originated (vs. not having one)

from nodebestpractices.

@benjamingr sounds great. I'll PR

from nodebestpractices.

There were several discussions in node core about this. It is mostly solved in recent V8 versions by the engine deducing a name for "anonymous" function based on their initial reference. i.e. see top stack frame:

> var x = () => {throw new Error()}
undefined
> x()
Error
    at x (repl:2:22)
    at repl:1:1
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at REPLServer.defaultEval (repl.js:239:29)
    at bound (domain.js:301:14)
    at REPLServer.runBound [as eval] (domain.js:314:12)
    at REPLServer.onLine (repl.js:440:10)
    at emitOne (events.js:120:20)
    at REPLServer.emit (events.js:210:7)
    at REPLServer.Interface._onLine (readline.js:282:10)
>

from nodebestpractices.

in recent V8 versions by the engine

It was actually added at the ES level

from nodebestpractices.

There is one limitation:

> setImmediate(() => {throw new Error()})
>
Error
    at Immediate.setImmediate [as _onImmediate] (repl:2:27)
    at runCallback (timers.js:785:20)
    at tryOnImmediate (timers.js:747:5)
    at processImmediate [as _immediateCallback] (timers.js:718:5)
>

When you do not control the first reference. For this you need to learn the name mapping. Maybe this should be documented.

from nodebestpractices.

Sounds indeed like an issue that should get addressed by tools and not by slowing down developers.

Question is how efficient is the deduction in real world scenario (function assignement seems like an easy case)

from nodebestpractices.

@refack what do u mean 'name mapping'?

from nodebestpractices.

That Immediate.setImmediate [as _onImmediate] is the name of the callback argument for setImmediate

> var a = setImmediate(function g() {throw new Error()})
undefined
> Error
    at Immediate.g [as _onImmediate] (repl:2:42)
    ...

> var a = setImmediate(() => {throw new Error()})
undefined
> Error
    at Immediate.setImmediate [as _onImmediate] (repl:2:35)
   ...
>

from nodebestpractices.

@refack in your example the tools provide a decent clue to the developer. Good enough to rely on it.

But take a look at this:

class Boo{
    loo(){
        setTimeout(()=> {throw new Error("Foo")}, 50);
    }
}

new Boo().loo()

Error: Foo
    at Timeout.setTimeout [as _onTimeout] (c:\code\playground\node_9ff4789d77136.tmp:5:32)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)

Note: no Boo and Loo in stacktrace

from nodebestpractices.

@refack all of that is true - but the advice is still correct in that naming functions in that case makes things easier.

Question for @i0natan though:

at Timeout.setTimeout [as _onTimeout] (c:\code\playground\node_9ff4789d77136.tmp:5:32)

Why does that not point to a real file? @i0natan Can you run it with --inspect and see what the results are like with a debugger attached?

from nodebestpractices.

@refack I'm aware of the difficulty in deducting the virtual stack the code has gone through (though I understand only 80% of it :), my point was not related to node/vs internals rather to the question - are the tools good enough to provide visibility into anonymous arrow functions. Should the hypothetical developer had named that callback with a named function, I believe he had a much better visibility into the error location. On the other hand, I hate verbosity, so I'm uncertain...

@benjamingr I use vscode 'Node Exec' for quick tryouts, this is the reason behind the weird file name.

REPL:

undefined
> Error: Foo
    at Timeout.setTimeout [as _onTimeout] (repl:1:54)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)
>

--Inspect:
C:\code\playground\test.js:5
setTimeout(()=> {throw new Error("Foo")}, 50);
^

Error: Foo
    at Timeout.setTimeout [as _onTimeout] (C:\code\playground\test.js:5:32)
    at ontimeout (timers.js:469:11)
    at tryOnTimeout (timers.js:304:5)
    at Timer.listOnTimeout (timers.js:264:5)

from nodebestpractices.

Interesting, so there is impact but only if there is no debugger.

from nodebestpractices.

@benjamingr probably I miss something - what differences do you see between the stacks? they all look similar to me

from nodebestpractices.

Correct file name, line and column which lets me know where the error originated (vs. not having one)

So IMHO this sums down to personal preference.

• Being verbose can help you deduce the identity of the callback by name - is the following example iNamedThisCBSoIcoundFindIt:

  named_callback.js

  1: 'use strict'
  2: const iNamedThisCBSoIcoundFindIt = () => {throw new Error()};
  3: setImmediate(iNamedThisCBSoIcoundFindIt)

  d:\temp$ node d:\code\4node\named_callback.js
  Error
      at Immediate.iNamedThisCBSoIcoundFindIt [as _onImmediate] (d:\code\4node\named_callback.js:2:49)
      at runCallback (timers.js:785:20)
      at tryOnImmediate (timers.js:747:5)
      at processImmediate [as _immediateCallback] (timers.js:718:5)

• Keeping the function anonymous still provides enough information via the location in the code:

  unnamed_callback.js

  1: 'use strict'
  2: setImmediate(() => {throw new Error()})

  d:\temp$ node d:\code\4node\unnamed_callback.js
  Error
      at Immediate.setImmediate [as _onImmediate] (d:\code\4node\unnamed_callback.js:2:27)
      at runCallback (timers.js:785:20)
      at tryOnImmediate (timers.js:747:5)
      at processImmediate [as _immediateCallback] (timers.js:718:5)

from nodebestpractices.

@refack yeah, the name thing advice is the most useful in client side apps where minification (without source maps) interferes with it a lot.

from nodebestpractices.

Errors have line numbers so locating the function is not a real issue. How about memory dump/flamegraph?

from nodebestpractices.

@i0natan flame graphs contain a reference to the actual function and if source maps are set correctly the inspector will handle it (I did it last week) but it does add more overhead.

from nodebestpractices.

@refack @benjamingr what do u suggest here as a reasonable and practical advice? we can't keep contradicting content

from nodebestpractices.

Hello there! 👋
This issue has gone silent. Eerily silent. ⏳
We currently close issues after 100 days of inactivity. It has been 90 days since the last update here.
If needed, you can keep it open by replying here.
Thanks for being a part of the Node.js Best Practices community! 💚

from nodebestpractices.