feat: add pathMapping to node.js

[jchip]

Thank you for pointing me to the pathMapping option.

This PR implements node.js support for the option.

Thank you for your time and consideration.

[ghost]

All CLA requirements met.

[jchip]

@connor4312 I am not sure why the windows failure. do you have any pointers?

[connor4312]

Thanks for the PR! I pushed some small tidying. I think there's two more things we may want to do:

1. Apply the path mapping when converting a path to a URL. This is used for setting breakpoints. Here it is in the browser resolver, we can pull that to a function or a shared method in the SourcePathResolverBase. (This could also be micro-optimized a bit if you feel like it)

   https://github.com/microsoft/vscode-js-debug/blob/69d988bc578bc4e5b987dcd8900e85e722eb1aa4/src/targets/browser/browserPathResolver.ts#L59-L63

2. Do we need to check await this.fsUtils.exists(mapped2) in rebaseRemoteWithPathMapping? Loading of sources is probably the hottest path in the debugger, any extra I/O we can avoid is worth avoiding. If we do that we can also drop the async from rebaseRemoteWithPathMapping, defaultPathMappingResolver doesn't actually await anything internally.

[jchip]

Thanks for looking into this.

For the exists check, I was thinking that if user maps a path by npm scope only, then some package may not have an original local copy, so checking if it exist first would help. For performance, since the check only occurs if a path maps, it should be limited in scope. Please let me know what you think.

I will look into #1. Thanks.

[jchip]

I looked into the code regarding 1 and 2. Some thoughts and questions:

1. Doing the mapping at absolutePathToUrl is more logical. Do we still need to do the path mapping occurring in sourceMapSourceToAbsolute? https://github.com/microsoft/vscode-js-debug/pull/1015/files#diff-5b6af565ae623d97161f91a7c674975da6700025764d8795109353c2bc5d5174R207

2. Because the path mapping is just doing a startsWith to check mapping match, it's a likely case the user specify a path that maps too many files with some that don't map to real files. We could say that the user needs to specify more specific mapping, but doing an exist check would definitely help. Performance wise, it may add some sub-second latency, but likely not anything noticeable even for thousands of files and should not cause serious bottleneck, so I think it's a reasonable trade-off.

[connor4312]

Hi, sorry for the late response.

1. For sourceMapSourceToAbsolute, I think we should do this. In your case, the debugger will see node_modules/foo/index.js is mapped to node_modules/foo/index.ts (referencing ./index.ts) so we would still need to map that to ../foo/index.ts.
2. "it's a likely case the user specify a path that maps too many files with some that don't map to real files" I don't think this is super common. A user would need to explicitly, for example, map /foo to some bad path. This is an intentional action, and if the path is bad I would rather not fall back to the unmapped path, since the "bad path" will still be visible in the debugger (e.g. if stepping into the source and in the Loaded Sources view) which the user can use to fix that path.

[jchip]

thanks.

for 2, the case I was thinking is, user could be mapping node_modules/@myscope to packages/myscope, because we do startWith when matching the maps, some packages in node_modules/@myscope may not come from packages/myscope, and that would end up with a bad mapped path, but yeah, it's not much of an issue, just that user needs to be well informed so they realize their mistake and explicitly spell out the mapping for each package under their npm scope.

[jchip]

@connor4312 I think I may have mistaken this to be working. I tried it again, while the pathMapping code is properly returning the mapped path, vscode is not opening the mapped file.

My guess is that the breakpoint predictor also needs to know about the pathMapping? It currently search all workspace for sourceMapURL and process them, so something similar also has to be done for pathMapping?

Could you give me some ideas please? Thanks.

[jchip]

@connor4312

• the breakpoints prediction needs update to account for path mappings also. latest commit fixes that. I've made the changes to piggy back on the sourceMap types a bit.
• While testing this, I found that if a module has mixed code files with and without sourcemap info, if we don't do the exist check, then if user path map that module, all sourcemap info are not loaded.

please review. thank you for your time and consideration.

[connor4312]

Hi, I started merging this in https://github.com/microsoft/vscode-js-debug/tree/jchip-path-mappings. I don't think we need the changes to the breakpoint predictor and instead manage it by updating absolutePathToUrlRegexp. Let me know if it works for you!

[jchip]

Hi, thanks for looking into this. I am not sure what to change in absolutePathToUrlRegexp to make it work. Can you give me some more details please?

[jchip]

i did a bit of tracing, here is what I found.

I have a test project that maps src/index.js to lib/index.js

1. Launch debug from test project with path mapping and a breakpoint set in the src/index.js
2. reach setBreakpoints in src/adapter/breakpoints.ts, params contain the file src/index.js and line
3. reach predictBreakpoints in src/adapter/breakpointPredictor.ts, and the changes that translate the breakpoint to the mapped file lib/index.js
4. reach absolutePathToUrlRegexp where argument absolutePath is src/index.js
5. vscode stops at the breakpoint in src/index.js

Without the changes to handle path mapping in step 3, vscode doesn't know that it needs to setup "mapped breakpoint" for lib/index.js so won't stop.

I am not sure how to achieve that in absolutePathToUrlRegexp without the prediction to map the breakpoints.