ci: Ability to open a browser when the debug session is created #315

letynsoft · 2018-10-12T19:49:41Z

Adds launch option which allows setting the command to run and the URL to open (to open a browser) with the xdebug query param to start the debugging session

codecov · 2018-10-12T20:47:45Z

Codecov Report

Merging #315 into master will decrease coverage by 0.31%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   70.05%   69.74%   -0.32%     
==========================================
  Files           5        5              
  Lines        1012     1028      +16     
  Branches      161      163       +2     
==========================================
+ Hits          709      717       +8     
- Misses        303      311       +8

Impacted Files	Coverage Δ
src/phpDebug.ts	`66.26% <37.5%> (-0.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 047777b...af056c8. Read the comment docs.

felixfbecker · 2018-10-13T14:21:22Z

package.json

+                "type": "object",
+                "description": "Allows you to open the browser with xdebug start query string",
+                "properties": {
+                  "cmd": "string",


I don't like that one has to specify a command, can we just use a package like https://www.npmjs.com/package/open?

Is this valid JSON schema? Should this be "cmd": {"type": "string"}?

felixfbecker · 2018-10-13T14:21:56Z

package.json

+                "description": "Allows you to open the browser with xdebug start query string",
+                "properties": {
+                  "cmd": "string",
+                  "URI": "string"


use camelCase

felixfbecker · 2018-10-13T14:22:35Z

src/phpDebug.ts

@@ -67,6 +67,9 @@ interface LaunchRequestArguments extends VSCodeDebugProtocol.LaunchRequestArgume
    /** XDebug configuration */
    xdebugSettings?: { [featureName: string]: string | number }

+    /** Ability to open browser */
+    openUrl?: { [key: string]: string }


This says that the object allows arbitrary properties, but the schema clearly says only cmd and URL are allowed

felixfbecker · 2018-10-13T14:23:06Z

src/phpDebug.ts

@@ -990,6 +996,11 @@ class PhpDebugSession extends vscode.DebugSession {
                    await connection.close()
                    this._connections.delete(id)
                    this._waitingConnections.delete(connection)
+                    if (this._args.openUrl && this._args.openUrl.cmd && this._args.openUrl.URI) {
+                        childProcess.spawn(this._args.openUrl.cmd, [


There is no error handling here

letynsoft · 2018-10-15T21:46:24Z

Hi, i will implement the other things you menthined, but when i try to add the open module it just wont create the packages-lock.jsosn file (which seems to be neerded for the checking stuff), can you help me with that (tried simple npm install --save open, but as i say, doesn't update the lock file)?
Also not sure what you mean with the camel case one, but i guess, once i figure out the open package, it will only need a string URL there

felixfbecker · 2018-10-15T21:48:36Z

Are you running at least node 8 and npm 6?

letynsoft · 2018-10-17T20:55:18Z

And of course it does update it now... :-/ so you were probably right and it didn;t work until reboot ... ok will do the other things now... so don't bother :)

Remove requirement for the cmd, change the config option from object to string and add some logging

Change the launch.json script of the test project to implement the latest changes

letynsoft · 2018-10-17T21:57:18Z

Hi,
So i have updated the feature.
Let me know your thoughts :)

felixfbecker · 2018-10-18T14:37:19Z

src/phpDebug.ts

@@ -12,6 +12,9 @@ import { Terminal } from './terminal'
 import { isSameUri, convertClientPathToDebugger, convertDebuggerPathToClient } from './paths'
 import minimatch = require('minimatch')

+let open = require('open')


Please use import open from 'open' or this is not type safe. You'll need to add npm install --save-dev @types/open

felixfbecker · 2018-10-18T14:37:46Z

src/phpDebug.ts

@@ -12,6 +12,9 @@ import { Terminal } from './terminal'
 import { isSameUri, convertClientPathToDebugger, convertDebuggerPathToClient } from './paths'
 import minimatch = require('minimatch')

+let open = require('open')
+let appendQuery = require('append-query')


This is built into Node, no need for a dependency. Just use import { URL } from 'url'

felixfbecker · 2018-10-18T14:46:04Z

src/phpDebug.ts

@@ -311,6 +317,12 @@ class PhpDebugSession extends vscode.DebugSession {
                    this.sendErrorResponse(response, <Error>error)
                })
                server.listen(args.port || 9000, (error: NodeJS.ErrnoException) => (error ? reject(error) : resolve()))
+                if (args.openUrl) {
+                    let proc = open(appendQuery(args.openUrl, 'XDEBUG_SESSION_START=vscode'))
+                    proc.stderr.on('data', (data: any) => {


data is not any, it's either a string or a Buffer, in which case the logged output would be useless (octets). I don't know from the top of my head what it is, but it should not be typed any.

felixfbecker · 2018-10-18T14:46:33Z

src/phpDebug.ts

+                if (args.openUrl) {
+                    let proc = open(appendQuery(args.openUrl, 'XDEBUG_SESSION_START=vscode'))
+                    proc.stderr.on('data', (data: any) => {
+                        this.sendEvent(new vscode.OutputEvent('openUrl: ' + util.inspect(data) + '\n'))


You shouldn't use inspect because it's not a structured object

felixfbecker · 2018-10-18T14:47:25Z

src/phpDebug.ts

+                proc.stderr.on('data', (data: any) => {
+                    this.sendEvent(new vscode.OutputEvent('openUrl: ' + util.inspect(data) + '\n'))
+                })
+            }


This code is duplicated, it should be in a shared function

felixfbecker · 2018-10-18T14:47:36Z

src/phpDebug.ts

@@ -311,6 +317,12 @@ class PhpDebugSession extends vscode.DebugSession {
                    this.sendErrorResponse(response, <Error>error)
                })
                server.listen(args.port || 9000, (error: NodeJS.ErrnoException) => (error ? reject(error) : resolve()))
+                if (args.openUrl) {
+                    let proc = open(appendQuery(args.openUrl, 'XDEBUG_SESSION_START=vscode'))


Use const

letynsoft · 2018-10-21T21:37:23Z

Hi,
Another update...
Seems i didn't notice the first open should have been opn in the commit title :-/

letynsoft · 2018-11-21T21:49:18Z

Any news here? :)

felixfbecker · 2018-11-22T08:04:27Z

package.json

@@ -34,10 +34,12 @@
    "url": "https://github.com/felixfbecker/vscode-php-debug/issues"
  },
  "dependencies": {
+    "append-query": "^2.0.1",


This dependency is now unused

felixfbecker · 2018-11-22T08:05:41Z

src/phpDebug.ts

+            })
+        }).catch((reason: any) => {
+            this.sendEvent(new vscode.OutputEvent('openUrl-error: ' + util.inspect(reason) + '\n'))
+        })


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

Not sure what exactly you mean here... so i should remove the .catch and just return the result?
Also should the then be removed too?
The start call would have to be moved to 'connection' callback, but that should be fine

See my two suggestions above. In addition you'd remove the .catch() and "unwrap" the .then().

felixfbecker · 2018-11-22T08:06:36Z

src/phpDebug.ts

+        }
+        const myurl = new url.URL(this._args.openUrl)
+        if (start) {
+            myurl.searchParams.append('XDEBUG_SESSION_START', 'vscode')


I think this should be set, not append, since append would duplicate the parameter if it’s already set

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

I don't understand what this has to do with the stop command - what I mean is that append would potentially add XDEBUG_SESSION_START twice or more times, like ?XDEBUG_SESSION_START=vscode&XDEBUG_SESSION_START=vscode, while set() would just override it so XDEBUG_SESSION_START appears exactly once

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.

.set('XDEBUG_SESSION_START', ...) won't remove the XDEBUG_SESSION_STOP param.

felixfbecker · 2018-12-22T23:56:10Z

src/phpDebug.ts

+     * Opens the urls specified in configuration, either with start or stop query string
+     * @param {boolean} start
+     */
+    private _openUrl(start: boolean) {


Suggested change

private _openUrl(start: boolean) {

private async _openUrl(start: boolean): Promise<void> {

felixfbecker · 2018-12-22T23:56:24Z

src/phpDebug.ts

+            myurl.searchParams.append('XDEBUG_SESSION_STOP', 'vscode')
+        }
+
+        const proc = opn(myurl.toString())


Suggested change

const proc = opn(myurl.toString())

const proc = await opn(myurl.toString())

ianef · 2019-03-08T18:43:00Z

Hi @letynsoft @felixfbecker
This is an excellent step forward. I've just tried to implement a simple solution using the "program" and "args" properties but it stops debugging when the "program" script ends.

Any idea on when you might get this completed, it's a feature I would really like to see. I've just moved back to VSC from NetBeans which had a pretty good config for doing this and I'm missing it already.

As an aside, could you also add something along the lines of "program" (very weird choice of parameter name for script) and "args" that does not terminate the debug session which it finishes? Maybe an "init" object with "program" or "script" and "args" that are run when the debugger is started. This would allow platform specific code to run via exec which could open a browser (start http://.... on Windows) or all manner of other funky stuff.

letynsoft added 4 commits October 12, 2018 21:46

ci: Ability to open a browser when the debug session is created

0a279b5

Adds launch option which allows setting the command to run and the URL to open (to open a browser) with the xdebug query param to start the debugging session

fix: Do not require adtional package

f4bfe85

fix: remove comment

d5c5de2

fix: Linting issues

9c0b9af

letynsoft added 3 commits October 12, 2018 23:19

fix: some code linting

448613c

fix: Only cancel the session when stop was requested

0f1206e

fix: another linting issue

be24c05

felixfbecker requested changes Oct 13, 2018

View reviewed changes

letynsoft added 3 commits October 17, 2018 23:24

fix: Tidy up the implementation

f781213

Remove requirement for the cmd, change the config option from object to string and add some logging

fix: Fix some more linting issues

a2ca114

fix: Fix the test project launch json

6fbb150

Change the launch.json script of the test project to implement the latest changes

felixfbecker requested changes Oct 18, 2018

View reviewed changes

fix: Switched to open (open is deprecated), refactor to a function

af056c8

felixfbecker reviewed Nov 22, 2018

View reviewed changes

felixfbecker reviewed Dec 22, 2018

View reviewed changes

felixfbecker changed the base branch from master to main December 21, 2020 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: Ability to open a browser when the debug session is created #315

ci: Ability to open a browser when the debug session is created #315

letynsoft commented Oct 12, 2018

codecov bot commented Oct 12, 2018 •

edited by ghost

felixfbecker Oct 13, 2018

felixfbecker Oct 13, 2018

felixfbecker Oct 13, 2018

felixfbecker Oct 13, 2018

felixfbecker Oct 13, 2018

letynsoft commented Oct 15, 2018

felixfbecker commented Oct 15, 2018

letynsoft commented Oct 17, 2018 •

edited

letynsoft commented Oct 17, 2018 •

edited

felixfbecker Oct 18, 2018

felixfbecker Oct 18, 2018

felixfbecker Oct 18, 2018

felixfbecker Oct 18, 2018

felixfbecker Oct 18, 2018

felixfbecker Oct 18, 2018

letynsoft commented Oct 21, 2018

letynsoft commented Nov 21, 2018

felixfbecker Nov 22, 2018

felixfbecker Nov 22, 2018

letynsoft Dec 22, 2018 •

edited

felixfbecker Dec 22, 2018

felixfbecker Nov 22, 2018

letynsoft Dec 22, 2018 •

edited

felixfbecker Dec 22, 2018

letynsoft Dec 25, 2018

felixfbecker Dec 25, 2018

felixfbecker Dec 22, 2018

felixfbecker Dec 22, 2018

ianef commented Mar 8, 2019 •

edited

	private _openUrl(start: boolean) {
	private async _openUrl(start: boolean): Promise<void> {

	const proc = opn(myurl.toString())
	const proc = await opn(myurl.toString())

ci: Ability to open a browser when the debug session is created #315

Are you sure you want to change the base?

ci: Ability to open a browser when the debug session is created #315

Conversation

letynsoft commented Oct 12, 2018

codecov bot commented Oct 12, 2018 • edited by ghost

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letynsoft commented Oct 15, 2018

felixfbecker commented Oct 15, 2018

letynsoft commented Oct 17, 2018 • edited

letynsoft commented Oct 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letynsoft commented Oct 21, 2018

letynsoft commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letynsoft Dec 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

letynsoft Dec 22, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ianef commented Mar 8, 2019 • edited

codecov bot commented Oct 12, 2018 •

edited by ghost

letynsoft commented Oct 17, 2018 •

edited

letynsoft commented Oct 17, 2018 •

edited

letynsoft Dec 22, 2018 •

edited

letynsoft Dec 22, 2018 •

edited

ianef commented Mar 8, 2019 •

edited