Skip to content
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: add pathMapping to node.js #1015

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

jchip
Copy link

@jchip jchip commented May 31, 2021

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
Copy link

ghost commented May 31, 2021

CLA assistant check
All CLA requirements met.

@connor4312 connor4312 added this to the June 2021 milestone Jun 1, 2021
@jchip
Copy link
Author

jchip commented Jun 2, 2021

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

@connor4312
Copy link
Member

connor4312 commented Jun 10, 2021

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)

    const defaultMapping = ['/', pathMapping['/']] as const;
    const bestMatch =
    Object.entries(pathMapping)
    .sort(([, directoryA], [, directoryB]) => directoryB.length - directoryA.length)
    .find(([, directory]) => isSubdirectoryOf(directory, absolutePath)) || defaultMapping;

  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.

@connor4312 connor4312 self-assigned this Jun 10, 2021
@jchip
Copy link
Author

jchip commented Jun 11, 2021

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
Copy link
Author

jchip commented Jun 14, 2021

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
Copy link
Member

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
Copy link
Author

jchip commented Jun 24, 2021

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
Copy link
Author

jchip commented Jun 24, 2021

@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
Copy link
Author

jchip commented Jul 15, 2021

@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
Copy link
Member

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!

@connor4312 connor4312 modified the milestones: June 2021, August 2021 Aug 5, 2021
@jchip
Copy link
Author

jchip commented Aug 16, 2021

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
Copy link
Author

jchip commented Aug 16, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants