Skip to content
This repository was archived by the owner on Feb 1, 2022. It is now read-only.

Conversation

@hybrist
Copy link
Collaborator

@hybrist hybrist commented Mar 16, 2017

Tested against nodejs/node#11431.

On versions of node that don't use the new protocol by default, this test is skipped (and the feature will most likely not work).

}

function runScript(script, scriptArgs, inspectHost, inspectPort, childPrint) {
return portIsFree(inspectHost, inspectPort)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moving the port check here so it's skipped for remote debugging. Waiting or the the port to be available doesn't make sense for node inspect <host>:<port>/node inspect -p <pid>.

port = parseInt(portMatch[1], 10);
script = args[0];
scriptArgs = args.slice(1);
} else if (args.length === 1 && /^\d+$/.test(args[0]) && target === '-p') {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much 1:1 copy of the existing _debugger.js code.

@hybrist
Copy link
Collaborator Author

hybrist commented Mar 16, 2017

/cc @nodejs/diagnostics
/cc @eugeneo (FYI)

@hybrist
Copy link
Collaborator Author

hybrist commented Apr 3, 2017

Manually tested rebased version against nodejs/node#11431, still works.

@hybrist
Copy link
Collaborator Author

hybrist commented Apr 4, 2017

@joshgav If you have some time, this is the last bit that still needs a review.

@hybrist hybrist requested a review from joshgav April 4, 2017 15:15
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM insofar I'm able to judge that.

@hybrist
Copy link
Collaborator Author

hybrist commented Apr 4, 2017

Thanks! Passes on latest nightly & 7.x, merging.

@hybrist hybrist merged commit 5341594 into master Apr 4, 2017
@hybrist hybrist deleted the jk-pid branch April 4, 2017 15:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants