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

inspector: normalized script URL in inspector protocol #22223

Closed
alexkozy opened this issue Aug 10, 2018 · 3 comments
Closed

inspector: normalized script URL in inspector protocol #22223

alexkozy opened this issue Aug 10, 2018 · 3 comments
Assignees
Labels
inspector Issues and PRs related to the V8 inspector protocol

Comments

@alexkozy
Copy link
Member

  • Version: each starting Node 6 when Node gets V8Inspector.
  • Platform: all.
  • Subsystem: inspector.

V8Inspector exposes protocol for inspection and debugging node state. There are a lot of different URL exposures:

Node internally uses the platform-specific path as script URL in some cases and file URL in other (see --experimental-modules). It makes using protocol much harder, e.g., when we need to set breakpoint ahead of time, we need to generate RegExp that will match platform-specific path and file URL to be ready for a different way how Node executes module.

I have a plan to make everything better:

  1. Add new V8InspectorClient method: resourceNameToURL. V8Inspector calls this method whenever getting a new URL from Node, e.g., when the new script is parsed.
  2. Implement this method for Node. Node implementation will normalize the platform-specific path to the file URL.
  3. Backport these two patches through Node 10 back to 8 since they should not affect anything in runtime and will make a life of different Node tools much easier.
@alexkozy alexkozy added the inspector Issues and PRs related to the V8 inspector protocol label Aug 10, 2018
@alexkozy alexkozy self-assigned this Aug 10, 2018
@TimothyGu
Copy link
Member

+1. This has caused some confusion in bcoe/c8#14, and I wouldn't surprised if other Inspector-based tools also meet this.

  1. Backport these two patches through Node 10 back to 8 since they should not affect anything in runtime and will make a life of different Node tools much easier.

This is the part I'm not sure about. I could certainly imagine existing tools using the format of the URL (that it's… not a URL) to distinguish Node.js instances from Chrome, for example. Changing the URL of an inspected script could be seen as a semver-major change even.

@alexkozy
Copy link
Member Author

distinguish Node.js instances from Chrome, for example

Are we talking about Electron case when Node.js and Chrome share the same V8? Otherwise, it sounds like we should have two different connection one to node and one to Chrome. In Elector like case Debugger.scriptParsed event contains an executionContextAuxData field that should include various information for Node scripts and Chrome scripts.

I believe that at least I can backport my V8 change to current master branch and start working on normalization for Node 11. At the same time I think that with current URLs it is not possible to write a tool that will work nicely with experimental modules and regular modules without processing file URL case, so most tools should be prepared, and it would be nice to get it merged to Node 10 to help tools developers.

@alexkozy
Copy link
Member Author

@TimothyGu
I have alternative to merging back proposal: merge it back to Node 8 but enable only behind the flag. WDYT?

I need it for ndb.

targos pushed a commit that referenced this issue Sep 19, 2018
This method is required by inspector to report normalized urls over
the protocol.

Backport-PR-URL: #22918
Fixes #22223
PR-URL: #22251
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
targos pushed a commit that referenced this issue Sep 25, 2018
This method is required by inspector to report normalized urls over
the protocol.

Backport-PR-URL: #22918
Fixes #22223
PR-URL: #22251
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
DominicKramer pushed a commit to googleapis/cloud-debug-nodejs that referenced this issue Nov 20, 2018
Newer versions of Node will require that URLs be well-formatted when setting breakpoints (and will also return well-formatted URLs), as a result of nodejs/node#22223 (this will also affect versions of Node to which that PR will be backported). Enforcing well-formatted URLs will break the Debug Agent, so this PR addresses that.

If tests pass for current versions of Node, I think this should be merged now (rather than wait for Node 11).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

No branches or pull requests

2 participants