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

Line 1 breakpoint locations may no longer be accurate after 10.16+ #737

Closed
kjin opened this issue Aug 14, 2019 · 3 comments · Fixed by #751
Closed

Line 1 breakpoint locations may no longer be accurate after 10.16+ #737

kjin opened this issue Aug 14, 2019 · 3 comments · Fixed by #751
Assignees
Labels
api: clouddebugger Issues related to the googleapis/cloud-debug-nodejs API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@kjin
Copy link
Contributor

kjin commented Aug 14, 2019

A failing test (testing setting line-1 breakpoints) that is the cause of #729 highlights a potentially breaking change for us in Node 10.16: scripts are now parsed slightly differently. Consider the following code:

module.exports = function foo(n) { return n; }

In Node 10.15 and below, getting the above script via Inspector's Debugger.getScriptSource yields:

(function (exports, require, module, __filename, __dirname) { module.exports = function foo(n) { return n; }
});

In Node 10.16+, it yields the original source:

module.exports = function foo(n) { return n; }

This almost certainly affects line-1 breakpoints.

@kjin kjin self-assigned this Aug 14, 2019
@kjin
Copy link
Contributor Author

kjin commented Aug 14, 2019

This issue should not be closed until the test skipped in #738 is re-enabled.

@kjin kjin added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Aug 14, 2019
@bcoe
Copy link
Contributor

bcoe commented Aug 16, 2019

@kjin interesting, I remember when this was introduced, compileFunction is now used:

https://github.com/nodejs/node/blob/master/lib/internal/modules/cjs/loader.js#L856

rather than source being pre-wrapped with a wrapper, this should ultimately help ensure slightly more accurate information, but it does mean that prior to Node.10.16 the wrapper exists afterwords it does not.

@kjin
Copy link
Contributor Author

kjin commented Aug 20, 2019

@bcoe Thanks, I found the PR that caused this change: nodejs/node#21573

@kjin kjin closed this as completed in #751 Aug 21, 2019
@google-cloud-label-sync google-cloud-label-sync bot added the api: clouddebugger Issues related to the googleapis/cloud-debug-nodejs API. label Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: clouddebugger Issues related to the googleapis/cloud-debug-nodejs API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants