-
-
Notifications
You must be signed in to change notification settings - Fork 178
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
ci: Ability to open a browser when the debug session is created #315
base: main
Are you sure you want to change the base?
Changes from all commits
0a279b5
f4bfe85
d5c5de2
9c0b9af
448613c
0f1206e
be24c05
f781213
a2ca114
6fbb150
af056c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -11,6 +11,7 @@ import * as fs from 'fs' | |||||
import { Terminal } from './terminal' | ||||||
import { isSameUri, convertClientPathToDebugger, convertDebuggerPathToClient } from './paths' | ||||||
import minimatch = require('minimatch') | ||||||
import opn = require('opn') | ||||||
|
||||||
if (process.env['VSCODE_NLS_CONFIG']) { | ||||||
try { | ||||||
|
@@ -67,6 +68,9 @@ interface LaunchRequestArguments extends VSCodeDebugProtocol.LaunchRequestArgume | |||||
/** XDebug configuration */ | ||||||
xdebugSettings?: { [featureName: string]: string | number } | ||||||
|
||||||
/** Ability to open browser */ | ||||||
openUrl?: string | ||||||
|
||||||
// CLI options | ||||||
|
||||||
/** If set, launches the specified PHP script in CLI mode */ | ||||||
|
@@ -311,6 +315,7 @@ class PhpDebugSession extends vscode.DebugSession { | |||||
this.sendErrorResponse(response, <Error>error) | ||||||
}) | ||||||
server.listen(args.port || 9000, (error: NodeJS.ErrnoException) => (error ? reject(error) : resolve())) | ||||||
this._openUrl(true) | ||||||
}) | ||||||
try { | ||||||
if (!args.noDebug) { | ||||||
|
@@ -325,6 +330,30 @@ class PhpDebugSession extends vscode.DebugSession { | |||||
} | ||||||
this.sendResponse(response) | ||||||
} | ||||||
/** | ||||||
* Opens the urls specified in configuration, either with start or stop query string | ||||||
* @param {boolean} start | ||||||
*/ | ||||||
private _openUrl(start: boolean) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
if (!this._args.openUrl) { | ||||||
return | ||||||
} | ||||||
const myurl = new url.URL(this._args.openUrl) | ||||||
if (start) { | ||||||
myurl.searchParams.append('XDEBUG_SESSION_START', 'vscode') | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be set, not append, since append would duplicate the parameter if it’s already set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would rather keep the append, as a the stop command would be confusing for the xdebug anyway and this may be visible straight away There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what this has to do with the stop command - what I mean is that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but what would happen when the stop command should be sent? XDEBUG_SESSION_START=vscode&XDEBUG_SESSION_STOP=vscode (with set or append) as the param is diferrent, so it would be easier to spot such errors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||
} else { | ||||||
myurl.searchParams.append('XDEBUG_SESSION_STOP', 'vscode') | ||||||
} | ||||||
|
||||||
const proc = opn(myurl.toString()) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
proc.then(child => { | ||||||
child.stderr.on('data', (chunk: string) => { | ||||||
this.sendEvent(new vscode.OutputEvent('openUrl: ' + chunk + '\n')) | ||||||
}) | ||||||
}).catch((reason: any) => { | ||||||
this.sendEvent(new vscode.OutputEvent('openUrl-error: ' + util.inspect(reason) + '\n')) | ||||||
}) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of loggint the error here, it’s better to make this an async function and have the callers await it, so an error is reported as a response to the request There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what exactly you mean here... so i should remove the .catch and just return the result? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my two suggestions above. In addition you'd remove the |
||||||
} | ||||||
|
||||||
/** | ||||||
* Checks the status of a StatusResponse and notifies VS Code accordingly | ||||||
|
@@ -992,6 +1021,7 @@ class PhpDebugSession extends vscode.DebugSession { | |||||
this._waitingConnections.delete(connection) | ||||||
}) | ||||||
) | ||||||
this._openUrl(false) | ||||||
// If listening for connections, close server | ||||||
if (this._server) { | ||||||
await new Promise(resolve => this._server.close(resolve)) | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dependency is now unused