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

cannonicalizing url so that it can find the matching script when resolving breakpoint #379

Merged
merged 7 commits into from Nov 16, 2018

Conversation

Projects
None yet
3 participants
@Shenniey
Copy link
Contributor

Shenniey commented Nov 12, 2018

the breakpoint is moving b/c it cannot match authored to generated path in node scenario: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/685518

Shennie Yao
cannonicalizing url so that it can find the matching script in attach…
… scenario (otherwise, the breakpoint moves)
@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Nov 12, 2018

I can see the upstream issue but that issue tracker isn't public, could you copy it into this repo?

Also, if you debugged this could you show why the url needs to be canonicalized here? Is this another file: URI case?

@Shenniey Shenniey changed the title cannonicalizing url so that it can find the matching script in attach scenario cannonicalizing url so that it can find the matching script when resolving breakpoint Nov 12, 2018

@Shenniey

This comment has been minimized.

Copy link
Contributor

Shenniey commented Nov 13, 2018

Hi, I'm sorry for doing this so late, but yes, I've added the issue I to this repo: #380

Yes, it's another file:URI case. The problem shows up in OnBreakpointsResolved, when it is looking for a matching breakpoint for breakpoints in routing files in commited breakpoints. The filepath in committed breakpoints in routing files is canonicalized (though the main app.js file is not canonicalized...)
committed breakpoints by url

But the url it is searching for has the file:/// prefix:

lookingfor script

I am going to make another commit because although in the instances I've seen so far, only breakpoints in routing files are searched for in OnBreakpointResolved (likely due to lazy loading, although I'm not 100% sure), so the fact that the main app.js file is not canonicalized has not caused any issues in my own testing. However, I'm not sure if that would come up as a problem anywhere else, so I am planning to change it so that it checks in the committed breakpoints for the given file path, and then checks for the canonicalized filepath if it cannot find the given one. Would that be a better approach?

let canonicalizedUrl = utils.canonicalizeUrl(script.url);
committedBps = this._committedBreakpointsByUrl.get(canonicalizedUrl);
if (committedBps) {
script.url = canonicalizedUrl;

This comment has been minimized.

@roblourens

roblourens Nov 14, 2018

Member

I'm not sure about mutating something that was already saved in scriptsById. If it should be canonicalized at this point, then that should be done at the point it was saved into scriptsById

@roblourens

This comment has been minimized.

Copy link
Member

roblourens commented Nov 14, 2018

Thanks for the details!

const committedBps = this._committedBreakpointsByUrl.get(script.url) || [];
// If we cannot find the script url in the committed breakpoints, we check for the canonicalized path. If found, use those committed breakpoints
// If we cannot find either the given url or the canonicalized url (or they are the same and the script simply cannot be found), committed breakpoints is an empty array
let committedBps = this._committedBreakpointsByUrl.get(script.url);

This comment has been minimized.

@digeff

digeff Nov 14, 2018

Contributor

If the scripts inside are canonicalized we should be able to just search for the canonicalized version

@Shenniey Shenniey closed this Nov 15, 2018

Shennie Yao

@Shenniey Shenniey reopened this Nov 15, 2018

Shennie Yao
@@ -1203,19 +1203,23 @@ export abstract class ChromeDebugAdapter implements IDebugAdapter {
return;
}

const committedBps = this._committedBreakpointsByUrl.get(script.url) || [];
// committed breakpoints (this._committedBreakpointsByUrl) should always have url keys in canonicalized form
const committedBps = this.getValueFromCommittedBreakpointsByUrl(script.url);

This comment has been minimized.

@digeff

digeff Nov 15, 2018

Contributor

What will happen if there is no commitedBps for this url?

This comment has been minimized.

@Shenniey

Shenniey Nov 15, 2018

Contributor

Sorry I forgot to put back the empty array

Shennie Yao added some commits Nov 15, 2018

@Shenniey Shenniey closed this Nov 15, 2018

@Shenniey Shenniey reopened this Nov 15, 2018

@roblourens roblourens merged commit 41d8254 into Microsoft:master Nov 16, 2018

1 of 2 checks passed

vscode-chrome-debug-core-CI #20181116.2 failed
Details
license/cla All CLA requirements met.
Details

@roblourens roblourens added this to the November 2018 milestone Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment