smartStep should only skip unmapped lines in files that have sourcemaps

[digeff]

It would be great if smartstepping would do something smart for JavaScript projects, or JavaScript files in a mixed JS/TS Project

[roblourens]

or JavaScript files in a mixed JS/TS Project

Original assumption for smartStep was that the sourcemapped code is the user code, and everything else should be skipped. But I can imagine someone who only wants to skip unmapped portions of files with sourcemaps, e.g. TS injected async/await code. Especially now that we have skipFiles for people who want to skip node internal files, etc.

A quicker fix for VS's specific issue, can you disable skipFiles when no .ts files are in the project?

[digeff]

I fixed VS by disabling smartstep, so we are not in a hurry. We'd like eventually to be able to use this feature in JS and mixed projects though...

[kkeri]

But I can imagine someone who only wants to skip unmapped portions of files with sourcemaps

I want to do this. I guess source maps become more and more important, so it's a good direction to improve their support, and smartstep is a logical step on this way. But don't make source maps viral.

For example, I can write my tests in pure JS, or I can work on interplaying packages, some of them written in pure JS, others transpiled. In that case I'd like to enjoy the benefit of skipping generated blablah, but I still want to visit my code.

[roblourens]

By default, smartStep should step over unmapped lines in a file that has a sourcemap, but not step through a .js file that doesn't have a sourcemap.

I can't decide whether we should rely on skipFiles for stepping through all other files - someone can add node_modules and node_internals and whatever else they need - or if smartStep should have a mode that gives it the current behavior. We could have "smartStep": "unmappedLines"/"unmappedFiles" or something like that. Thoughts @weinand?

[roblourens]

@digeff I want to make this change in vscode to see what feedback we get

By default, smartStep should step over unmapped lines in a file that has a sourcemap, but not step through a .js file that doesn't have a sourcemap.

Do you enable smartStep for VS? I think that's the behavior you want but I wouldn't want to break anything for you before a release.

[digeff]

Currently smartStep is disabled in VS.
We'd love to start using this functionality in the future, we just need to figure out the best way to do it.

Feel free to change this anyway you need.

[roblourens]

I'd like to fix this for June.

smartStep could become a complex property with multiple modes. A mode for skipping unmapped lines in files with sourcemaps (async/await in TS), a mode for lines in unmapped files (e.g. node internals or node_modules, although this is better served by using skipFiles) and we can leave it open for other modes in the future. Another might be rejected promises that incorrectly break as if they are uncaught exceptions in some cases (we have a checkbox in the breakpoints view for these in the EH debugger).

With this change, smartStep can look like this:

"smartStep": {
  "unmappedLines": true,            // skip unmapped lines in files with sourcemaps (the async/await TS case)
  "filesWithoutSourcemaps": false   // skip lines in files without sourcemaps
}

Which would let me skip async/await code injected by TS or babel, but not skip .js files in node_modules. This is the usecase that is really useful for smartStep currently.

For backcompat with users' existing configs, "smartStep": true would have the same behavior as it does currently. This would be equivalent to

"smartStep": {
  "unmappedLines": true,
  "filesWithoutSourcemaps": true
}

Then after a month or so of stabilization, I'd like to enable this "unmappedLines" mode by default. People expect to step through their source files without randomly seeing generated lines in that same file, and they file issues on this on occasion. Debugging with smart step for these cases is a much nicer experience.

Questions -

• How critical is it to keep the behavior of "smartStep": true the same as it has been?
  • It would be nicer to say that "smartStep": true would only enable the "unmappedLines". But there are probably lots of people with that property set who would see a behavior change. Or, it might not be a big deal.
• Any suggestions for better names for the two smartStep properties?
• Is it confusing/overkill that "filesWithoutSourcemaps" exists, when skipFiles also exists? They can cover different cases but there is a lot of overlap in their scenarios. skipFiles is what we should push people towards (like with node_modules/**/*.js or <node_internals>).

Thoughts @weinand, @auchenberg, @isidorn?

[roblourens]

Ping

[kkeri]

@roblourens I welcome your proposal. It would perfectly fit my use case with "filesWithoutSourcemaps": false. However I can't speak for those who prefer the current behavior.

[auchenberg]

Quick feedback @roblourens

1. Do we know how many that's using "smartStep": true? If the usage is reasonably low I don't have problem of slowly deprecating it. Users can update their configs, and if they don't we will degrade the experience, not disrupt it.

2. I like the naming. It's explicit, which is good.

3. Isn't his different than skipFiles which ignores files completely even if they have breakpoints in then, where filesWithoutSourcemaps removes them for the stack frames?

[roblourens]

skipFiles will actually still hit breakpoints. It's the same as 'blackbox scripts' in Chrome devtools. skipFiles targets files with a pattern, filesWithoutSourcemaps would target files that don't have sourcemaps.

[weinand]

@roblourens I think we should change the semantics of the existing "smartStep": true to only cover files with sourcemaps. I think that is more what people expect the semantics to be, given that we have "skipFiles" too (which could easily cover the unmapped files case).

So we could just change the implementation (and document this) without changing any launch config attributes.

However, if we are planning to add more options to the "smartStep" attribute, then your approach of turning it into a structured thing makes sense too.

The attributes "unmappedLines" and "filesWithoutSourcemaps" appear to be very different (shape of the words) but semantically they are actually quite close. Would it make sense to use "unmappedFiles" instead of "filesWithoutSourcemaps"?

"smartStep": {
  "unmappedLines": true,  // skip unmapped lines in files with sourcemaps
  "unmappedFiles": true   // skip files without sourcemaps
}

But in light of the first observation that "unmappedFiles" is already covered by the "skipFiles" feature, I think another option is to drop it altogether which leaves us with this strange thing:

"smartStep": {
  "unmappedLines": true,  // skip unmapped lines in files with sourcemaps
}

I suggest that we either

• add the "rejectedPromises" option so that there are at least two attributes (and the introduction of a structure is justified) or
• we leave the "smartStep" attribute as a boolean for now, but change and document the semantics, and make "smartStep": true the default (and gather feedback).

And if we want to add an "rejectedPromises" option in the future, we can still turn "smartStep" into a structure.

What do you think?

[roblourens]

Even though skipFiles exists, it seems like skipping all files without sourcemaps is still a good heuristic for debugging user code, in many cases. And since people may be relying on that, I'm still nervous about changing what "smartStep": true does without a clear way forward. I think that discovering how to do "skipFiles": ["<node_internals>/**"] is not easy, maybe that's another problem.

On the other hand, "smartStep": true should enable whatever the recommended configuration is. So maybe I change it in Insiders the way you describe, and see what complaints come in.

Side note, maybe we could have other smartStep attributes like "nodeInternals": true which would essentially just register the above node_internals skipFiles option. It would be more discoverable because of intellisense. But we'd also have two ways to do the same thing, maybe better to figure out how to educate people about skipFiles.

Also I'm thinking that "rejectedPromises" really belongs as a checkbox next to the exception breakpoints options, where it already is for EH debugging.

[roblourens]

What I'm leaning towards right now is... just change "smartStep": true to use the "unmappedLines" behavior, and see whether anyone complains.

I think "skip all code without sourcemaps" is still a good heuristic for debugging the user's code, but it also includes things like whatever random node_module includes sourcemaps. I think we should push people towards skipFiles for cases that smartStep won't cover.

So the only question is how to educate people about the change. I can leave a note in the smartStep description and maybe I need an explainer question on SO or just an issue in the vscode repo.

[weinand]

@roblourens I will change he smartStep behavior for node-debug legacy too (microsoft/vscode-node-debug#182).

you've raised the question how to document the change:

• release notes
• smartStep documentation: https://code.visualstudio.com/docs/nodejs/nodejs-debugging#_smart-stepping. We should explicitly mention to use skipFiles for anything that is not covered by smartStep.
• StackOverflow: yes, maybe...

[weinand]

I've pushed the change for node-debug (legacy).