fix: correct column numbers for line-1 breakpoints (#751)

googleapis · Aug 21, 2019 · f6d4f76 · f6d4f76
1 parent 7143525
commit f6d4f76
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 28 deletions.
diff --git a/src/agent/v8/inspector-debugapi.ts b/src/agent/v8/inspector-debugapi.ts
@@ -42,6 +42,17 @@ interface InspectorOptions {
   useWellFormattedUrl: boolean;
 }
 
+/**
+ * In older versions of Node, the script source as seen by the Inspector
+ * backend is wrapped in `require('module').wrapper`, and in new versions
+ * (Node 10.16+, Node 11.11+, Node 12+) it's not. This affects line-1
+ * breakpoints.
+ */
+const USE_MODULE_PREFIX = utils.satisfies(
+  process.version,
+  '<10.16 || >=11 <11.11'
+);
+
 export class BreakpointData {
   constructor(
     public id: inspector.Debugger.BreakpointId,
@@ -405,10 +416,9 @@ export class InspectorDebugApi implements debugapi.DebugApi {
     const line = mapInfo
       ? mapInfo.line
       : (breakpoint.location as stackdriver.SourceLocation).line;
-    // We need to special case breakpoints on the first line. Since Node.js
-    // wraps modules with a function expression, we adjust
-    // to deal with that.
-    if (line === 1) {
+    // In older versions of Node, since Node.js wraps modules with a function
+    // expression, we need to special case breakpoints on the first line.
+    if (USE_MODULE_PREFIX && line === 1) {
       column += debugapi.MODULE_WRAP_PREFIX_LENGTH - 1;
     }
 

diff --git a/test/test-v8debugapi.ts b/test/test-v8debugapi.ts
@@ -1819,30 +1819,29 @@ describe('v8debugapi', () => {
       });
     });
 
-    // Skipped until #737 is resolved.
-    // it('should correctly stop on line-1 breakpoints', done => {
-    //   const foo = require('./fixtures/foo.js');
-    //   // TODO(dominickramer): Have this actually implement Breakpoint
-    //   const bp: stackdriver.Breakpoint = ({
-    //     id: 'bp-line-1',
-    //     location: {path: 'foo.js', line: 1, column: 45},
-    //   } as {}) as stackdriver.Breakpoint;
-    //   api.set(bp, err1 => {
-    //     assert.ifError(err1);
-    //     api.wait(bp, err2 => {
-    //       assert.ifError(err2);
-    //       assert.ok(bp.stackFrames);
-
-    //       api.clear(bp, err3 => {
-    //         assert.ifError(err3);
-    //         done();
-    //       });
-    //     });
-    //     process.nextTick(() => {
-    //       foo();
-    //     });
-    //   });
-    // });
+    it('should correctly stop on line-1 breakpoints', done => {
+      const foo = require('./fixtures/foo.js');
+      // TODO(dominickramer): Have this actually implement Breakpoint
+      const bp: stackdriver.Breakpoint = ({
+        id: 'bp-line-1',
+        location: {path: 'foo.js', line: 1, column: 45},
+      } as {}) as stackdriver.Breakpoint;
+      api.set(bp, err1 => {
+        assert.ifError(err1);
+        api.wait(bp, err2 => {
+          assert.ifError(err2);
+          assert.ok(bp.stackFrames);
+
+          api.clear(bp, err3 => {
+            assert.ifError(err3);
+            done();
+          });
+        });
+        process.nextTick(() => {
+          foo();
+        });
+      });
+    });
 
     it('should not silence errors thrown in the wait callback', done => {
       const message = 'This exception should not be silenced';