Use webRoot config for source path resolution #885
Conversation
bwalderman
force-pushed
the
user/brwalder/webRoot
branch
from
271ce6d to
0769ab8
Compare
bwalderman
requested review from
hkal and
antross
and removed request for
vidorteg
src/utils.ts
Outdated
| @@ -471,6 +471,11 @@ export function getRuntimeConfig(config: Partial<IUserConfig> = {}): IRuntimeCon | |||
| } | |||
| } | |||
|
|
|||
| // The webRoot config is equivalent to a pathMapping entry of { '/': '${webRoot}' }. | |||
| if (!pathMapping.hasOwnProperty('/')) { | |||
There was a problem hiding this comment.
This isn't working for me because I'm getting a pathMapping object back from the settings.get('pathMapping') call that already has {'/': '${workspaceFolder}'}. However, I don't have that set in my settings.json file so I'm still trying to figure out where it's coming from.
@mliao95 any idea what might be causing this?
There was a problem hiding this comment.
Not off the top of my head. I'll dig into it
There was a problem hiding this comment.
@bwalderman I think even once we figure out what's going on with settings.get this may not work as expected since the fallback SETTINGS_DEFAULT_PATH_MAPPING also specifies a root entry.
There was a problem hiding this comment.
Sorry - got sidetracked, not sure if you already figured it out, but the default value for pathMapping is {'/': '${workspaceFolder}'} as defined in the package.json:
https://github.com/microsoft/vscode-edge-devtools/blob/main/package.json#L217
There was a problem hiding this comment.
I had not figured that out - thanks!
@mliao95 any thoughts on what a correct fix would be here given we're often already going to have a default / specified?
- Should we give precedence to
webRootdepending on where it originates from? - Or give precedence if it's anything other than our default?
- Or should we abandon this change and advise users to set a
pathMappingfor/themselves?
There was a problem hiding this comment.
In that case I think we should always give precedence to webRoot. The default webRoot is {$workspaceFolder}, so this will not affect the default pathMapping unless the user has specified something else for webRoot.
There was a problem hiding this comment.
IMO I think the priority based off of my interpretation of user expectations should be:
User defined extension settings > webRoot > default setting value.
Let me know if you agree and we can run with that. Otherwise, open to hear your thoughts.
Edit:
On second thought, I'm okay webRoot taking precedence over the extension settings. The difference is pretty minor and removes some complexity in the priority logic.
There was a problem hiding this comment.
Thanks @mliao95. Based on the issue report, I think user expectations are that existing launch configs with webRoot properties just work with our extension the way they do for vscode JS debugging sessions. On top of that, the launch config should take priority over the extension setting because any logic on the vscode-js-debug side won't even be aware of our extension settings.
Fixes #787
The
webRootconfig was not being considered when mapping a URL to a path on the filesystem. This prevented file links in the devtools from successfully opening files in the editor. The fix is to treatwebRootas an entry inpathMapping.This fixes the
webRootproperty in Microsoft Edge DevTools' own extension settings, but the extension isn't currently aware ofwebRootandpathMappingproperties found inlaunch.jsonconfigs. A separate PR microsoft/vscode-js-debug#1206 plumbs those through to our extension so they will work as expected.