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

Commit

Permalink
fix: line numbers don't change in transpiled files (#436)
Browse files Browse the repository at this point in the history 
Fixes: #417
  • Loading branch information
@DominicKramer
DominicKramer committed Jun 12, 2018
1 parent 71ae615 commit 2d7a0f7
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 24 deletions.
3 changes: 2 additions & 1 deletion src/agent/io/sourcemapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,8 @@ export class SourceMapper {
}

const sourcePos = {
source: path.relative(path.dirname(entry.mapFile), inputPath),
source: path.relative(path.dirname(entry.mapFile), inputPath)
.replace(/\\/g, '/'),
line: lineNumber + 1, // the SourceMapConsumer expects the line number
// to be one-based but expects the column number
column: colNumber // to be zero-based
Expand Down
10 changes: 10 additions & 0 deletions src/agent/util/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -207,3 +207,13 @@ export function satisfies(nodeVersion: string, semverRange: string) {
const finalVersion = coercedVersion ? coercedVersion.version : nodeVersion;
return semver.satisfies(finalVersion, semverRange);
}

/**
* Used to determine if the specified file is a JavaScript file
* by determining if it has a `.js` file extension.
*
* @param filepath The path of the file to analyze.
*/
export function isJavaScriptFile(filepath: string) {
return path.extname(filepath).toLowerCase() === '.js';
}
3 changes: 2 additions & 1 deletion src/agent/v8/inspector-debugapi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -466,7 +466,8 @@ export class InspectorDebugApi implements debugapi.DebugApi {
const captured = state.capture(
callFrames, breakpoint, this.config, this.scriptMapper,
this.v8Inspector);
if (breakpoint.location) {
if (breakpoint.location &&
utils.isJavaScriptFile(breakpoint.location.path)) {
breakpoint.location.line = callFrames[0].location.lineNumber + 1;
}
breakpoint.stackFrames = captured.stackFrames;
Expand Down
5 changes: 3 additions & 2 deletions src/agent/v8/legacy-debugapi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,6 @@ export class V8DebugApi implements debugapi.DebugApi {
const column = 0;
const mapInfo =
this.sourcemapper.mappingInfo(baseScriptPath, line, column);

const compile = utils.getBreakpointCompiler(breakpoint);
if (breakpoint.condition && compile) {
try {
Expand Down Expand Up @@ -455,7 +454,9 @@ export class V8DebugApi implements debugapi.DebugApi {
// TODO: Address the case where `breakpoint.expression` is `undefined`.
const captured = state.capture(
execState, breakpoint.expressions as string[], this.config, this.v8);
if (breakpoint.location && captured.location && captured.location.line) {
if (breakpoint.location &&
utils.isJavaScriptFile(breakpoint.location.path) &&
captured.location && captured.location.line) {
breakpoint.location.line = captured.location.line;
}
breakpoint.stackFrames = captured.stackFrames;
Expand Down
29 changes: 29 additions & 0 deletions test/test-v8debugapi-ts-code.ts
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/* 1 TESTS RELY ON THE PRECISE LINE NUMBERS IN THIS FILE */
/* 2*/ export interface Point {
/* 3*/ x: number;
/* 4*/ y: number;
/* 5*/ }
/* 6*/
/* 7*/ export function dist(pt1: Point, pt2: Point) {
/* 8*/ const xdiff = pt1.x - pt2.x;
/* 9*/ const ydiff = pt1.y - pt2.y;
/*10*/ const pnorm1 = Math.abs(xdiff) + Math.abs(ydiff);
/*11*/ const pnorm2 = Math.sqrt(xdiff * xdiff + ydiff * ydiff);
/*12*/ return {pnorm1, pnorm2};
/*13*/ }

/*
* Copyright 2018 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the 'License');
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an 'AS IS' BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
86 changes: 66 additions & 20 deletions test/test-v8debugapi.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ import * as stackdriver from '../src/types/stackdriver';
// TODO(dominickramer): Have this actually implement Breakpoint
const breakpointInFoo: stackdriver.Breakpoint = {
id: 'fake-id-123',
// TODO(dominickramer): Determine if we should be restricting to only the build directory.
// TODO(dominickramer): Determine if we should be restricting to only the
// build directory.
location: {path: 'build/test/test-v8debugapi-code.js', line: 5}
} as stackdriver.Breakpoint;

Expand All @@ -42,6 +43,7 @@ import * as path from 'path';
import * as utils from '../src/agent/util/utils';
import {debugAssert} from '../src/agent/util/debug-assert';
const code = require('./test-v8debugapi-code.js');
import {dist} from './test-v8debugapi-ts-code';

function stateIsClean(api: DebugApi): boolean {
assert.equal(
Expand Down Expand Up @@ -165,7 +167,8 @@ describe('debugapi selection', () => {
SourceMapper.create(mapFiles, (err, mapper) => {
assert(!err);
// TODO(dominickramer): Handle the case when mapper is undefined.
// TODO(dominickramer): Handle the case when v8debugapi.create returns null
// TODO(dominickramer): Handle the case when v8debugapi.create
// returns null
api = debugapi.create(
logger, config, jsStats,
mapper as SourceMapper.SourceMapper) as DebugApi;
Expand Down Expand Up @@ -218,7 +221,8 @@ describeFn('debugapi selection on Node >=10', () => {
describe('v8debugapi', () => {
const config: ResolvedDebugAgentConfig = extend(
{}, defaultConfig, {workingDirectory: __dirname, forceNewAgent_: true});
// TODO(dominickramer): It appears `logLevel` is a typo and should be `level`. However,
// TODO(dominickramer): It appears `logLevel` is a typo and should be `level`.
// However,
// with this change, the tests fail. Resolve this.
const logger =
new common.logger({levelLevel: config.logLevel} as {} as LoggerOptions);
Expand All @@ -235,7 +239,8 @@ describe('v8debugapi', () => {
assert(!err1);

// TODO(dominickramer): Handle the case when mapper is undefined.
// TODO(dominickramer): Handle the case when v8debugapi.create returns null
// TODO(dominickramer): Handle the case when v8debugapi.create
// returns null
api = debugapi.create(
logger, config, jsStats,
mapper as SourceMapper.SourceMapper) as DebugApi;
Expand Down Expand Up @@ -663,7 +668,7 @@ describe('v8debugapi', () => {
});
});

it('should resolve actual line number hit rather than originally set',
it('should resolve actual line number hit rather than originally set for js files',
(done) => {
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
Expand All @@ -685,6 +690,32 @@ describe('v8debugapi', () => {
});
});

it('should not change line number when breakpoints hit for transpiled files',
(done) => {
const bp: stackdriver.Breakpoint = {
id: 'fake-id-125',
location: {
path: path.join('test', 'test-v8debugapi-ts-code.ts'),
line: 10
}
} as stackdriver.Breakpoint;
api.set(bp, (err1) => {
assert.ifError(err1);
api.wait(bp, (err2) => {
assert.ifError(err2);
assert(bp.location);
assert.equal(bp.location!.line, 10);
api.clear(bp, (err3) => {
assert.ifError(err3);
done();
});
});
process.nextTick(() => {
dist({x: 1, y: 2}, {x: 3, y: 4});
});
});
});

it('should work with multiply hit breakpoints', (done) => {
const oldWarn = logger.warn;
let logCount = 0;
Expand Down Expand Up @@ -795,15 +826,17 @@ describe('v8debugapi', () => {
assert.ok(topFrame);
assert.equal(topFrame['function'], 'foo');
assert.equal(topFrame.arguments.length, 1);
// TODO(dominickramer): Handle the case when topFrame.arguments[0].varTableIndex
// TODO(dominickramer): Handle the case when
// topFrame.arguments[0].varTableIndex
// is undefined.
const argsVal =
bp.variableTable[topFrame.arguments[0].varTableIndex as number];
assert(argsVal!.status!.isError);
assert(argsVal!.status!.description.format.match(
'Locals and arguments are only displayed.*config.capture.maxExpandFrames=0'));
assert.equal(topFrame.locals.length, 1);
// TODO(dominickramer): Handle the case when topFrame.locals[0].varTableIndex is
// TODO(dominickramer): Handle the case when
// topFrame.locals[0].varTableIndex is
// undefined.
const localsVal =
bp.variableTable[topFrame.locals[0].varTableIndex as number];
Expand Down Expand Up @@ -910,7 +943,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file has
// been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 10},
expressions: ['process.env', 'hasGetter']
Expand Down Expand Up @@ -956,7 +990,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: breakpointInFoo.id,
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file has
// been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['A']
Expand Down Expand Up @@ -994,7 +1029,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file has
// been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 10}
} as {} as stackdriver.Breakpoint;
Expand Down Expand Up @@ -1035,7 +1071,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file has
// been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6}
} as {} as stackdriver.Breakpoint;
Expand Down Expand Up @@ -1070,7 +1107,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file has
// been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6}
} as {} as stackdriver.Breakpoint;
Expand Down Expand Up @@ -1106,7 +1144,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 10},
expressions: ['hasGetter']
Expand Down Expand Up @@ -1146,7 +1185,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['A']
Expand Down Expand Up @@ -1183,7 +1223,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['B']
Expand Down Expand Up @@ -1219,7 +1260,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['A']
Expand Down Expand Up @@ -1256,7 +1298,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['B']
Expand Down Expand Up @@ -1293,7 +1336,8 @@ describe('v8debugapi', () => {
// TODO(dominickramer): Have this actually implement Breakpoint
const bp: stackdriver.Breakpoint = {
id: 'fake-id-124',
// TODO(dominickramer): This path can be lest strict when this file has been
// TODO(dominickramer): This path can be lest strict when this file
// has been
// converted to Typescript.
location: {path: 'build/test/test-v8debugapi-code.js', line: 6},
expressions: ['A']
Expand Down Expand Up @@ -1732,11 +1776,13 @@ describe('v8debugapi', () => {

describe('v8debugapi.findScripts', () => {
it('should properly handle appPathRelativeToRepository', () => {
// TODO(dominickramer): `config` was used before it was defined and passed as the third
// TODO(dominickramer): `config` was used before it was defined and passed
// as the third
// parameter below. This was a Typescript compile error. The
// value of `undefined` should be functionally equivalent.
// Make sure that is the case.
// TODO(dominickramer): The third argument should be of type Object (not undefined).
// TODO(dominickramer): The third argument should be of type Object (not
// undefined).
// Fix this.
const config = extend(true, {}, undefined!, {
workingDirectory: '/some/strange/directory',
Expand Down

0 comments on commit 2d7a0f7

Please sign in to comment.