-
Notifications
You must be signed in to change notification settings - Fork 119
Add public interface to set targetFilter on connection at runtime. #350
Conversation
(Another thing I wasn't sure how to do: contribute 13 translations documenting the new property in the |
Thanks! I will review this soon... |
Thanks 😄 BTW, I checked out all the dependent packages listed in this project's README and determined only |
I agree that breaking other packages is probably not a big concern, thanks for doing that research. I wonder whether we need ChromeTargetDiscoveryStrategy to know about these targetTypes, when it already has targetFilter which is basically just used by chrome-debug to accomplish the same thing. Is it possible to just pass in a targetFilter which filters to the correct target types? I can't remember whether I told you to do this one way or another last time around. |
src/chrome/chromeDebugAdapter.ts
Outdated
|
||
// Filter down to a set of requested target types. Note that `targetTypes` might be `undefined` here, which is expected; | ||
// the result will be disabling target type filtering unless explicitly requested by the consumer. | ||
this._chromeConnection.setTargetTypes(args.targetTypes); |
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.
Yeah, I think we could pull it off without much friction…we'd just need to expose a public method like this one, so that ChromeConnection, in addition to possibly receiving targetFilter
at construction time, also can receive target filter at args-time. This block would roughly change to:
if (args.targetFilter) {
this._chromeConnection.setTargetFilter(args.targetFilter);
}
You want me to go with that approach instead?
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.
(cc: @roblourens)
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.
Yeah, I think that makes sense.
Okay @roblourens, done. PR title is updated to reflect this change, as is my fork of |
Thanks! |
Please see
vscode-chrome-debug
#713 for original (rejected) proposal for this feature.I wanted to emphasize backward compatibility when designing the API, so I opted to leave
targetFilter
alone (it's not broken and might be useful for other extensions ofvscode-chrome-debug-core
). Instead, where necessary, I:targetTypes
as the last optional argument at the tail end of existing chains of optional arguments.setTargetTypes(targetTypes: string[])
as a public method onChromeConnection
. This felt like a good alternative to rewiring the whole mechanism by whichChromeConnection
receivestargetFilter
for consistency. (Note: I would have used an ES6 setter here, but apparently TypeMoq has some trouble with setters in strict mode.)A corresponding
vscode-chrome-debug
pull request is blocked by this PR, but already done/tested in my fork here. If this checks out, would you please let me know the instructions for contributing sibling PRs for projects that usevscode-chrome-debug-core
? Despite the backward-compatible API, other packages that mockChromeConnection
in strict mode will probably need something like: