GeezerVison

Fixes: #41346

Or at least, it's one of the possible approaches. There are two we can do:

1. Keep current behavior, document as undefined.
2. Throw an exception and document as such.

v8.writeHeapSnapshot() calls the following code:

As we can see, the lack of args.GetReturnValue().Set(...) when WriteSnapshot(isolate, *name) returns false (lines 377 and 387) makes the function always return undefined when the file fails to write.

It does seem that the upstream module sets an error, which can be a lot more useful: https://github.com/bnoordhuis/node-heapdump/blob/3537b67cebdbd602d8572fcc533f223ed25e5cbc/src/heapdump.cc#L93-L96

I believe node-heapdump's approach is more sensible, as there are several reasons why a file write can fail (out of space, missing permissions...) and said error helps us determine the cause, where an undefined only tells us that the write failed.

The usage of v8.writeHeapSnapshot() is quite small in the ecosystem (according to GitHub's Code Search, 16 usages in JavaScript and 5 in TypeScript outside of Node.js forks) all of them rely on it returning a string or to write a file successfully, none of the matches seem to compare against undefined or a falsy/truthy value.

I don't know what would be the exact implication of defining the error state as an error being thrown rather than silently erroring with undefined, however, one of the matches was from DefinitelyTyped, a file used in CI:

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/2aae4db6bbe2e078c0cbc84cc9cd43cb1e01d8fd/types/node/v14/test/v8.ts#L13

I believe we are still able to change this behavior to throw an exception because:

• Low adoption in the (open-source) ecosystem* despite the function existing for three years.
• Developers seem to assume the write never fails or that it always returns a string*.
• The error state is not documented (and typically, changes in undocumented features usually don't fall into semantic versioning), if we merge this PR, changing the behavior to throwing an exception would result on a breaking change, which we are not allowed to do if we want to keep upwards compatibility.

*: According to cs.github.com's data.

As recommended by @RaisinTen in the aforementioned issue, I'm opening a docs PR. I would have opted in for making a PR for Approach 2 (throw an exception) instead if I had more availability and I was more confident to contribute in the code.

I'll be more than happy if somebody else can make the PR if the team decides to take the second approach, otherwise this PR should have everything for the first one. I checked all other heap snapshot methods, and none seems to have this behavior-documentation mismatch.