-
Notifications
You must be signed in to change notification settings - Fork 289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: fix pathways to allow debugging hot-transpiled modules #174
Conversation
Previously, this extension fared poorly if one attempted to debug a Node script that had in-place transpilation going on (e.g. @babel/register or ts-node). There were a few reasons for this. The most trivial one is that we didn't do content hashing for Node script as a performance optimization. Unfortuantely, we can't assume that any file on disk is actually what we're running in this scenario :) A second trivial one is that the source map filtering we added didn't support source map data URIs. We added a filter to ignore source maps coming from node_modules or outside the workspace folder, and we just didn't have logic for matching data URIs there. Now, if we see a data URI, we'll just treat the embedding file as the source map location for comparison purposes. The bulk of this commit deals with the third change. By default, we eagerly throw out breakpoints that match lines the exact file path or URI. This becomes problematic because the transpilation happens in-place, generating compiled sources that live appear at the same paths as the source files. The approach we take here is that if we see a source map which lands breakpoints in a file for which we've previously set breakpoints by URL, then remove the URL-set breakpoints in favor of the sourcemapped breakpoints. The bulk of changes were around resiliency for this, since breakpoints become a little more dynamic now in some cases: - We store a list of 'applying' breakpoints in addition to 'applied' ones, which we're able to remove or deadletter (e.g. if we run into a sourcemap while still setting a URL BP) - We used to store only a single UI location for the breakpoint, we now keep track of them and will change the location if a breakpoint is moved or adjusted. - Fixed a (probably rare) case where a BP could leak if a the thread changed while the breakpoint was being applied.
Hmm... The whole idea of transpiler transforming file in place sounds terrible. This means we loose the ability to debug either the original file, or the transformed one. Can we engage with @babel/register and/or other projects which do this kind of thing? IIUC, the easy fix for them would be to additionally include sourceUrl in the generated file which points to a different location, so that we don't even map generated file to non-matching source file. WDYT? |
I don't disagree, it is somewhat terrible, but it's been common practice in Node since the first transpilers were born... I'll prototype and follow up with the maintainers of common tools in a few days, but this'll be a scenario we need to support for a good while regardless. |
Following Rob's suggestion of what the v1 adapter does. When we set a breakpoint in a file, we'll also set a breakpoint on the first line that we consider to be an instrumentation breakpoint. We treat like the normal `beforeScriptWithSourceMapExecution` breakpoint, waiting breakpoints to be set (which in the case of babel/ts-node, includes any necessary adjustment) before continuing on. This commit also makes a tweak (over the existing changes in the PR) to avoid mapping breakpoints by path if we see our source is mapped in-place. The check for this is rather ugly, but it works.
b279e29
to
b566724
Compare
b566724
to
4eac739
Compare
1ba0747
to
11e025b
Compare
11e025b
to
dcf125d
Compare
Test flake seems unrelated |
First of all, apologies for the churn. While this change is relatively simple, I realized that hit condition breakpoints further diverges the user-defined breakpoints and the managed 'entrypoint' breakpoints (as of #174), where we don't have hit conditions or DAP representations. Therefore, there's now a base "Breakpoint" class which implements the purely CDP-side of things, which a `UserDefinedBreakpoint` extends with metadata around DAP, and the `EntryBreakpoint` extends in a minimal way. After doing the split there was almost no changes the existing code around the breakpoints, which is good evidence that we are indeed dealing with two different types of things! That done, there's a new `HitBreakpoint` class whose parsing logic is copied from the existing debug adapters. The UserDefinedBreakpoint optionally includes a HitBreakpoint, and we check on these when we hit a location to see whether we should automatically continue, or not. This works in my quick manual tests. Haven't had a chance to write unit tests for this, but I wanted to get it in PR before I head out for the holidays.
First of all, apologies for the churn. While this change is relatively simple, I realized that hit condition breakpoints further diverges the user-defined breakpoints and the managed 'entrypoint' breakpoints (as of #174), where we don't have hit conditions or DAP representations. Therefore, there's now a base "Breakpoint" class which implements the purely CDP-side of things, which a `UserDefinedBreakpoint` extends with metadata around DAP, and the `EntryBreakpoint` extends in a minimal way. After doing the split there was almost no changes the existing code around the breakpoints, which is good evidence that we are indeed dealing with two different types of things! That done, there's a new `HitBreakpoint` class whose parsing logic is copied from the existing debug adapters. The UserDefinedBreakpoint optionally includes a HitBreakpoint, and we check on these when we hit a location to see whether we should automatically continue, or not. This works in my quick manual tests. Haven't had a chance to write unit tests for this, but I wanted to get it in PR before I head out for the holidays.
* feat: initial implementation of hit condition breakpoints First of all, apologies for the churn. While this change is relatively simple, I realized that hit condition breakpoints further diverges the user-defined breakpoints and the managed 'entrypoint' breakpoints (as of #174), where we don't have hit conditions or DAP representations. Therefore, there's now a base "Breakpoint" class which implements the purely CDP-side of things, which a `UserDefinedBreakpoint` extends with metadata around DAP, and the `EntryBreakpoint` extends in a minimal way. After doing the split there was almost no changes the existing code around the breakpoints, which is good evidence that we are indeed dealing with two different types of things! That done, there's a new `HitBreakpoint` class whose parsing logic is copied from the existing debug adapters. The UserDefinedBreakpoint optionally includes a HitBreakpoint, and we check on these when we hit a location to see whether we should automatically continue, or not. This works in my quick manual tests. Haven't had a chance to write unit tests for this, but I wanted to get it in PR before I head out for the holidays. * fixup! entrypoint breakpoint behavior Added fixes and unit tests while looking into #204. - Our nice new entrypoint breakpoint interfered with user-defined breakpoints. CDP errors if we try to set multiple breakpoints at the same locations, so if we see the user has a BP on the first line, we omit setting our automatic breakpoint. We also tweak behavior to treat any first-line breakpoint as an entrypoint breakpoint. - We were unnecessarily deduplicating breakpoint application requests which caused breakpoints which resolved to the exact same location in transpiled code to not get set. * fixup! only retry in ci * fixup! update the test assertions now that offsets are fixed
Previously, this extension fared poorly if one attempted to debug a Node
script that had in-place transpilation going on (e.g. @babel/register or
ts-node). There were a few reasons for this.
The most trivial one is that we didn't do content hashing for Node
script as a performance optimization. Unfortuantely, we can't assume
that any file on disk is actually what we're running in this scenario :)
A second trivial one is that the source map filtering we added didn't
support source map data URIs. We added a filter to ignore source maps
coming from node_modules or outside the workspace folder, and we just
didn't have logic for matching data URIs there. Now, if we see a data
URI, we'll just treat the embedding file as the source map location for
comparison purposes.
The bulk of this commit deals with the third change. By default, we
eagerly throw out breakpoints that match lines the exact file path or
URI. This becomes problematic because the transpilation happens
in-place, generating compiled sources that live appear at the same paths
as the source files. The approach we take here is that if we see a
source map which lands breakpoints in a file for which we've previously
set breakpoints by URL, then remove the URL-set breakpoints in favor of
the sourcemapped breakpoints.
The bulk of changes were around resiliency for this, since breakpoints
become a little more dynamic now in some cases:
ones, which we're able to remove or deadletter (e.g. if we run into a
sourcemap while still setting a URL BP)
keep track of them and will change the location if a breakpoint is
moved or adjusted.
changed while the breakpoint was being applied.
Fixes #127