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

fix: line numbers don't change in transpiled files #436

Merged
merged 16 commits into from
Jun 12, 2018
Merged

fix: line numbers don't change in transpiled files #436

merged 16 commits into from
Jun 12, 2018

Conversation

DominicKramer
Copy link
Contributor

Fixes: #417

@DominicKramer DominicKramer requested a review from a team June 11, 2018 17:01
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 11, 2018
@DominicKramer
Copy link
Contributor Author

PTAL

jinwoo
jinwoo reviewed Jun 11, 2018
View reviewed changes
Copy link
Contributor

@jinwoo jinwoo left a comment

Choose a reason for hiding this comment

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

Sorry I don't really understand how debug agent works, but why are we dealing with both .ts files and .js files?

src/agent/v8/inspector-debugapi.ts Outdated
@@ -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 &&
breakpoint.location.path.toLowerCase().endsWith('.js')) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/test-v8debugapi.ts
@@ -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);

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

test/test-v8debugapi.ts Outdated
@@ -685,6 +690,30 @@ describe('v8debugapi', () => {
});
});

it.only(

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@googlebot googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Jun 11, 2018
@googlebot googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Jun 11, 2018
@DominicKramer
Copy link
Contributor Author

@jinwoo Both .ts and .js files are used because, to provide a nice user experience, breakpoints can be set in .ts files. In this case, sourcemap information is used to set the breakpoint in the associated .js file.

jinwoo
jinwoo approved these changes Jun 11, 2018
View reviewed changes
src/agent/v8/inspector-debugapi.ts Outdated
@@ -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 &&
path.extname(breakpoint.location.path).toLowerCase() === '.js') {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jinwoo
Copy link
Contributor

jinwoo commented Jun 11, 2018

@DominicKramer I understand that .ts files are used to provide a better user experience and I totally agree with that. What I don't understand is why/how stackdriver gives us both a .ts file path and a .js file path when a user sets a breakpoint.

jinwoo
jinwoo reviewed Jun 11, 2018
View reviewed changes
test/test-v8debugapi.ts Outdated
@@ -685,6 +690,29 @@ describe('v8debugapi', () => {
});
});

it.only('should not change line number when breakpoints hit for transpiled files',

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@DominicKramer DominicKramer merged commit 2d7a0f7 into googleapis:master Jun 12, 2018
@DominicKramer DominicKramer mentioned this pull request Jul 11, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jinwoo jinwoo jinwoo approved these changes

Assignees
No one assigned
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@DominicKramer @jinwoo @googlebot