-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Separate debug configuration providers from debug adapter contributors #10910
Conversation
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.
Regarding the testing steps, all worked for me.
- onDebug appears for any event.
- onDebugInitialConfigurations appears when no launch.json + add config + markdown open.
- onDebugResolve appears when running any configuration.
- onDebugResolve:node appears when running a node configuration.
- both onDebugResolve:* activation plugins are called.
- multiple calls are not made to each provider for each resolution.
Regarding the code, it looks good for the most part. I had some comments about the location of some interfaces and a change to the DebugService
interface, and some questions about some of the implementation details that mostly come from the old code.
if (pluginProviders.length === 0) { | ||
return this.delegated.provideDebugConfigurations(debugType, workspaceFolderUri); | ||
} | ||
} | ||
|
||
async provideDynamicDebugConfigurations(): Promise<{ type: string, configurations: DebugConfiguration[] }[]> { | ||
const result: Promise<{ type: string, configurations: theia.DebugConfiguration[] }>[] = []; | ||
|
||
for (const [type, contributor] of this.contributors.entries()) { | ||
const typeConfigurations = this.resolveDynamicConfigurationsForType(type, contributor); | ||
result.push(typeConfigurations); | ||
let results: DebugConfiguration[] = []; | ||
for (const provider of pluginProviders) { | ||
const result = await provider.provideDebugConfigurations(workspaceFolderUri); | ||
if (result) { | ||
results = results.concat(result); | ||
} | ||
} | ||
|
||
return Promise.all(result); | ||
return results; |
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 is another place where I'm not sure I understand what's going on. Since we may return this.delegated.provideDebugConfigurations
, we believe that it's possible that the non-plugin debug system may have information that the plugin system doesn't, but we only use it if the plugin system definitely knows nothing about the relevant type. Shouldn't we use what the non-plugin system knows even if the plugin system also has some information about the type in question? Put another way, why do we give precedence to plugins and exclude Theia extensions in this case?
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.
That's a very good question Colin !
I have just kept the logic that existed before this change,
as the non-plugin and plugin contributions have been so far separated by an else
statement, this logic exists from the first version of this file.
It is a very good topic for discussion, although it seems out of the scope of this change. Do you agree?
const map = await this.debugConfigurationManager.provideDynamicDebugConfigurations(); | ||
for (const [type, dynamicConfigurations] of map) { |
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 looks like the only place that this method is used, so I'm not sure about the need for the change from {type: string, configurations: DebugConfiguration[]}
to Map<string, DebugConfiguration[]>
. Apart from the breaking change, I wouldn't mind, but since the base DebugService
lives on the backend and is called from the frontend, the return types of its interface should be easily serializable types, and Map
s don't implement a toJSON
method, so they can't easily be transmitted between the backend and the frontend.
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.
The return type in the base was confusing i.e. "
{ type: string, configurations: DebugConfiguration[] }[]"
it indicates the possibility to have more than one object for the same 'type'.
In the other hand, the consolidated list from all the providers does not actually cross the 'json-rpc' interface, this is resolved in browser code under the 'debug-plugin-service.ts' see:
This method will be used once more under the open PR providing support for dynamic configurations on the debug view, so I think it's good to rectify the meaning before its use propagates further.
Let me know what you think !
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.
It's true that the existing type doesn't ensure that all DebugConfigurations
for a given heading are actually grouped together, and it's true that the only actual implementation of the method currently lives in the browser, but I'd still prefer to avoid adding a type to the interface that absolutely couldn't be implemented by the base DebugService
on the backend. Given that, Record<string, DebugConfiguration[] | undefined>
would achieve the goal of grouping configurations while retaining serializibility.
export interface DebugConfigurationProvider { | ||
readonly handle: number; | ||
readonly type: string; | ||
readonly triggerKind: DebugConfigurationProviderTriggerKind; | ||
provideDebugConfigurations?(folder: string | undefined): Promise<DebugConfiguration[]>; | ||
resolveDebugConfiguration?(folder: string | undefined, debugConfiguration: DebugConfiguration): Promise<DebugConfiguration | undefined>; | ||
resolveDebugConfigurationWithSubstitutedVariables?(folder: string | undefined, debugConfiguration: DebugConfiguration): Promise<DebugConfiguration | undefined>; | ||
} | ||
|
||
export interface DebugConfigurationProviderDescriptor { | ||
readonly handle: number, | ||
readonly type: string, | ||
readonly trigger: DebugConfigurationProviderTriggerKind, | ||
readonly provideDebugConfiguration: boolean, | ||
readonly resolveDebugConfigurations: boolean, | ||
readonly resolveDebugConfigurationWithSubstitutedVariables: boolean | ||
} |
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.
Both of these interfaces are only used in the plugin-ext
package. Are they relevant to the 'internal' debugging system?
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.
Good point!,
I have moved them under the plugin-api-rpc.ts
file as it already contained a definition for 'DebugConfigurationProviderTriggerKind' and keeps then under the same plugin-ext
package.
for (const provider of pluginProviders) { | ||
const result = await provider.provideDebugConfigurations(workspaceFolderUri); | ||
if (result) { | ||
results = results.concat(result); |
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.
Minor: I think results.push(...result)
would be a little better than creating a new array each time.
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.
Makes sense !! Done!
return { type, configurations }; | ||
const configurationsMap = new Map<string, DebugConfiguration[]>(); | ||
|
||
await Promise.all(Array.from(pluginProviders, async provider => { |
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.
Minor:
await Promise.all(Array.from(pluginProviders, async provider => { | |
await Promise.all(pluginProviders.map(async provider => { |
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.
Done, thanks !
const results = this.configurationProviders.filter(p => p.handle === handle); | ||
if (results.length === 0) { | ||
throw new Error('No Debug configuration provider found with given handle number: ' + handle); | ||
} | ||
const { provider, type } = results[0]; |
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.
Minor:
const results = this.configurationProviders.filter(p => p.handle === handle); | |
if (results.length === 0) { | |
throw new Error('No Debug configuration provider found with given handle number: ' + handle); | |
} | |
const { provider, type } = results[0]; | |
const record = this.configurationProviders.find(p => p.handle === handle); | |
if (!record) { | |
throw new Error('No Debug configuration provider found with given handle number: ' + handle); | |
} | |
const { provider, type } = record; |
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.
Done, thanks !
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.
Addressing review comments, and reflected the updates in the latest commit!
const map = await this.debugConfigurationManager.provideDynamicDebugConfigurations(); | ||
for (const [type, dynamicConfigurations] of map) { |
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.
The return type in the base was confusing i.e. "
{ type: string, configurations: DebugConfiguration[] }[]"
it indicates the possibility to have more than one object for the same 'type'.
In the other hand, the consolidated list from all the providers does not actually cross the 'json-rpc' interface, this is resolved in browser code under the 'debug-plugin-service.ts' see:
This method will be used once more under the open PR providing support for dynamic configurations on the debug view, so I think it's good to rectify the meaning before its use propagates further.
Let me know what you think !
export interface DebugConfigurationProvider { | ||
readonly handle: number; | ||
readonly type: string; | ||
readonly triggerKind: DebugConfigurationProviderTriggerKind; | ||
provideDebugConfigurations?(folder: string | undefined): Promise<DebugConfiguration[]>; | ||
resolveDebugConfiguration?(folder: string | undefined, debugConfiguration: DebugConfiguration): Promise<DebugConfiguration | undefined>; | ||
resolveDebugConfigurationWithSubstitutedVariables?(folder: string | undefined, debugConfiguration: DebugConfiguration): Promise<DebugConfiguration | undefined>; | ||
} | ||
|
||
export interface DebugConfigurationProviderDescriptor { | ||
readonly handle: number, | ||
readonly type: string, | ||
readonly trigger: DebugConfigurationProviderTriggerKind, | ||
readonly provideDebugConfiguration: boolean, | ||
readonly resolveDebugConfigurations: boolean, | ||
readonly resolveDebugConfigurationWithSubstitutedVariables: boolean | ||
} |
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.
Good point!,
I have moved them under the plugin-api-rpc.ts
file as it already contained a definition for 'DebugConfigurationProviderTriggerKind' and keeps then under the same plugin-ext
package.
for (const provider of pluginProviders) { | ||
const result = await provider.provideDebugConfigurations(workspaceFolderUri); | ||
if (result) { | ||
results = results.concat(result); |
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.
Makes sense !! Done!
if (pluginProviders.length === 0) { | ||
return this.delegated.provideDebugConfigurations(debugType, workspaceFolderUri); | ||
} | ||
} | ||
|
||
async provideDynamicDebugConfigurations(): Promise<{ type: string, configurations: DebugConfiguration[] }[]> { | ||
const result: Promise<{ type: string, configurations: theia.DebugConfiguration[] }>[] = []; | ||
|
||
for (const [type, contributor] of this.contributors.entries()) { | ||
const typeConfigurations = this.resolveDynamicConfigurationsForType(type, contributor); | ||
result.push(typeConfigurations); | ||
let results: DebugConfiguration[] = []; | ||
for (const provider of pluginProviders) { | ||
const result = await provider.provideDebugConfigurations(workspaceFolderUri); | ||
if (result) { | ||
results = results.concat(result); | ||
} | ||
} | ||
|
||
return Promise.all(result); | ||
return results; |
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.
That's a very good question Colin !
I have just kept the logic that existed before this change,
as the non-plugin and plugin contributions have been so far separated by an else
statement, this logic exists from the first version of this file.
It is a very good topic for discussion, although it seems out of the scope of this change. Do you agree?
return { type, configurations }; | ||
const configurationsMap = new Map<string, DebugConfiguration[]>(); | ||
|
||
await Promise.all(Array.from(pluginProviders, async provider => { |
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.
Done, thanks !
const results = this.configurationProviders.filter(p => p.handle === handle); | ||
if (results.length === 0) { | ||
throw new Error('No Debug configuration provider found with given handle number: ' + handle); | ||
} | ||
const { provider, type } = results[0]; |
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.
Done, thanks !
Hi @colin-grant-work , |
8dca7bb
to
986e792
Compare
986e792
to
c001e24
Compare
This PR has just been re-based to the latest master |
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.
Just did a high level pass.
packages/plugin-ext/src/main/browser/debug/plugin-debug-service.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/browser/debug/plugin-debug-service.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/browser/debug/plugin-debug-service.ts
Outdated
Show resolved
Hide resolved
Updated in the latest commit!! Thanks !! |
@paul-marechal , |
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.
All of the provided plugins work as expected. @paul-marechal, if you'd give it another pass, I'd appreciate it - it LGTM.
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.
Just a few remarks, but the logic looks good.
packages/plugin-ext/src/main/browser/debug/plugin-debug-service.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/browser/debug/plugin-debug-configuration-provider.ts
Outdated
Show resolved
Hide resolved
packages/plugin-ext/src/main/browser/debug/plugin-debug-configuration-provider.ts
Outdated
Show resolved
Hide resolved
I have addressed all your comments except for the optional one which is an alternative I considered but I preferred the current one. |
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.
Code LGTM
The implementation of the vscode API for the resolution of initial debug configurations as well as the resolution of specific configurations was relying on the registered debug adapter contributors rather than debug configuration providers, this is causing issues e.g. #9978. This implementation brings access to the debug configuration providers all the way to the browser debug/plugin-debug-service, where the providers can be filtered and then call its corresponding API indirectly. The former API is kept as deprecated. Signed-off-by: Alvaro Sanchez-Leon <[email protected]>
04193c2
to
8997745
Compare
What it does
Fixes: #9978
The implementation of the vscode API for the resolution of initial
debug configurations as well as the resolution of specific
configurations was relying on registered debug adapter
contributors rather than debug configuration providers,
this is causing issues e.g. #9978.
This implementation brings access to the debug configuration
providers all the way to the browser debug/plugin-debug-service,
where the providers can be filtered and then call its corresponding
API indirectly towards the extension host.
The former API is kept and marked deprecated.
How to test
Based on: #5645
Use the test extension in the
tar
file as follows:https://github.com/alvsan09/debug-activation-events/raw/master/plugins.tar
NOTE: It is recommended to install only the required test extension(s), otherwise not all notifications may appear (must likely due to a hard coded limit).
onDebug
: install on-debug-0.0.1.vsix and check thatonDebug
notification appears on any debug activation eventonDebugInitialConfigurations
:onDebugInitialConfigurations
andprovideDebugConfigurations
notifications plus mock debug configuration should be generated dynamicallyonDebugResolve
:onDebugResolve
andresolveDebugConfiguration(*)[0]
notificationsonDebugResolve:<debug type>
:onDebugResolve:node
andresolveDebugConfiguration:node
notificationsonDebugResolve
:resolveDebugConfiguration(*)[0]
andresolveDebugConfiguration(*)[1]
notifications, one for each of the installedon-debug-resolve-*
extensions.Review checklist
Reminder for reviewers