Skip to content
This repository has been archived by the owner on Apr 3, 2024. It is now read-only.

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

Merged
merged 2 commits into from
Aug 21, 2019
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
19c4e6a
fix: correct column numbers for line-1 breakpoints
kjin Aug 20, 2019
c3957df
Merge branch 'master' into line-1
kjin Aug 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/agent/v8/inspector-debugapi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically did the same thing in one of my libraries.

https://github.com/istanbuljs/v8-to-istanbul/blob/master/lib/v8-to-istanbul.js#L11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I did see that v8-to-istanbul was mentioned in the Node PR that made this change, though I didn't realize it was your library 👍

);

export class BreakpointData {
constructor(
public id: inspector.Debugger.BreakpointId,
Expand Down Expand Up @@ -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;
}

Expand Down
47 changes: 23 additions & 24 deletions test/test-v8debugapi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down