update util.getCallSites() implementation to follow node.js (#3381)

Author: anonrig

src/node/internal/util.d.ts

@@ -121,6 +121,6 @@ export function isBoxedPrimitive(
 ): value is number | string | boolean | bigint | symbol;
 
 export function getBuiltinModule(id: string): any;
-export function getCallSite(frames: number): Record<string, string>[];
+export function getCallSites(frames?: number): Record<string, string>[];
 export function processExitImpl(code: number): void;
 export const processPlatform: string;

src/node/util.ts

@@ -206,9 +206,12 @@ export function deprecate(
   return fn;
 }
 
-export function getCallSite(frames: number = 10) {
-  return utilImpl.getCallSite(frames);
-}
+// Node.js originally introduced the API with the name `getCallSite()` as an experimental
+// API but then renamed it to `getCallSites()` soon after. We had already implemented the
+// API with the original name in a release. To avoid the possibility of breaking, we export
+// the function using both names.
+export const getCallSite = utilImpl.getCallSites.bind(utilImpl);
+export const getCallSites = utilImpl.getCallSites.bind(utilImpl);
 
 export function isDeepStrictEqual(a: unknown, b: unknown): boolean {
   return _isDeepStrictEqual(a, b);
@@ -247,6 +250,7 @@ export default {
   transferableAbortController,
   transferableAbortSignal,
   getCallSite,
+  getCallSites,
   isDeepStrictEqual,
   isArray,
 };

src/workerd/api/node/tests/util-nodejs-test.js

@@ -4343,15 +4343,23 @@ export const debuglog = {
   },
 };
 
-export const getCallSiteTest = {
+export const getCallSitesTest = {
   test() {
-    const callSites = util.getCallSite();
+    const callSites = util.getCallSites();
     assert.strictEqual(callSites.length, 1);
     const [stack] = callSites;
     assert.strictEqual(stack.functionName, 'test');
     assert.strictEqual(stack.scriptName, 'worker');
     assert.strictEqual(typeof stack.lineNumber, 'number');
+    assert.strictEqual(typeof stack.columnNumber, 'number');
     assert.strictEqual(typeof stack.column, 'number');
+
+    // We leave this implementation for compat reasons.
+    // Node.js originally introduced the API with the name `getCallSite()` as an experimental
+    // API but then renamed it to `getCallSites()` soon after. We had already implemented the
+    // API with the original name in a release. To avoid the possibility of breaking, we export
+    // the function using both names.
+    assert.strictEqual(typeof util.getCallSite, 'function');
   },
 };
 

src/workerd/api/node/util.c++

@@ -177,22 +177,29 @@ jsg::Name UtilModule::getResourceTypeInspect(jsg::Lock& js) {
   return js.newApiSymbol("kResourceTypeInspect"_kj);
 }
 
-kj::Array<UtilModule::CallSiteEntry> UtilModule::getCallSite(jsg::Lock& js, int frames) {
-  JSG_REQUIRE(
-      frames >= 1 && frames <= 200, Error, "Frame count should be between 1 and 200 inclusive."_kj);
-  auto stack = v8::StackTrace::CurrentStackTrace(js.v8Isolate, frames + 1);
+kj::Array<UtilModule::CallSiteEntry> UtilModule::getCallSites(
+    jsg::Lock& js, jsg::Optional<int> frames) {
+  KJ_IF_SOME(f, frames) {
+    JSG_REQUIRE(f >= 1 && f <= 200, Error, "Frame count should be between 1 and 200 inclusive."_kj);
+  }
+
+  auto stack = v8::StackTrace::CurrentStackTrace(js.v8Isolate, frames.orDefault(10) + 1);
   const int frameCount = stack->GetFrameCount();
   auto objects = kj::Vector<CallSiteEntry>();
   objects.reserve(frameCount - 1);
 
-  // Frame 0 is node:util. It should be skipped.
-  for (int i = 1; i < frameCount; ++i) {
+  for (int i = 0; i < frameCount; ++i) {
     auto stack_frame = stack->GetFrame(js.v8Isolate, i);
 
     objects.add(CallSiteEntry{
       .functionName = js.toString(stack_frame->GetFunctionName()),
       .scriptName = js.toString(stack_frame->GetScriptName()),
       .lineNumber = stack_frame->GetLineNumber(),
+      // Node.js originally implemented the experimental API using the "column" field
+      // then later renamed it to columnNumber. We had already implemented the API
+      // using column. To ensure backwards compat without the complexity of a compat
+      // flag, we just export both.
+      .columnNumber = stack_frame->GetColumn(),
       .column = stack_frame->GetColumn(),
     });
   }

src/workerd/api/node/util.h

@@ -212,11 +212,16 @@ class UtilModule final: public jsg::Object {
     kj::String functionName;
     kj::String scriptName;
     int lineNumber;
+    // Node.js originally introduced the API with the name `getCallSite()` as an experimental
+    // API but then renamed it to `getCallSites()` soon after. We had already implemented the
+    // API with the original name in a release. To avoid the possibility of breaking, we export
+    // the function using both names.
+    int columnNumber;
     int column;
 
-    JSG_STRUCT(functionName, scriptName, lineNumber, column);
+    JSG_STRUCT(functionName, scriptName, lineNumber, columnNumber, column);
   };
-  kj::Array<CallSiteEntry> getCallSite(jsg::Lock& js, int frames);
+  kj::Array<CallSiteEntry> getCallSites(jsg::Lock& js, jsg::Optional<int> frames);
 
 #define V(Type) bool is##Type(jsg::JsValue value);
   JS_UTIL_IS_TYPES(V)
@@ -257,7 +262,7 @@ class UtilModule final: public jsg::Object {
     JSG_METHOD(getProxyDetails);
     JSG_METHOD(previewEntries);
     JSG_METHOD(getConstructorName);
-    JSG_METHOD(getCallSite);
+    JSG_METHOD(getCallSites);
 
 #define V(Type) JSG_METHOD(is##Type);
     JS_UTIL_IS_TYPES(V)