Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Add public interface to set targetFilter on connection at runtime. #350

Merged
merged 1 commit into from
Sep 10, 2018

Conversation

stristr
Copy link
Contributor

@stristr stristr commented Sep 5, 2018

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 of vscode-chrome-debug-core). Instead, where necessary, I:

  • Added targetTypes as the last optional argument at the tail end of existing chains of optional arguments.
  • Exposed setTargetTypes(targetTypes: string[]) as a public method on ChromeConnection. This felt like a good alternative to rewiring the whole mechanism by which ChromeConnection receives targetFilter 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 use vscode-chrome-debug-core? Despite the backward-compatible API, other packages that mock ChromeConnection in strict mode will probably need something like:

        mockChromeConnection
            .setup(x => x.setTargetTypes(It.isValue(undefined)))
            .verifiable(Times.atLeast(0));

@stristr
Copy link
Contributor Author

stristr commented Sep 5, 2018

(Another thing I wasn't sure how to do: contribute 13 translations documenting the new property in the package.nls.*.json files 😄…)

@roblourens
Copy link
Member

Thanks! I will review this soon...

@stristr
Copy link
Contributor Author

stristr commented Sep 6, 2018

Thanks 😄

BTW, I checked out all the dependent packages listed in this project's README and determined only vscode-chrome-debug will need changes. Others are deprecated, track a much older version of this dependency, or lack a test that would break due to the reasons listed in the PR description.

@roblourens
Copy link
Member

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.


// 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);
Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(cc: @roblourens)

Copy link
Member

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.

@stristr stristr changed the title Add backward-compatible core option to filter targets by type. Add public interface to set targetFilter on connection at runtime. Sep 10, 2018
@stristr
Copy link
Contributor Author

stristr commented Sep 10, 2018

Okay @roblourens, done. PR title is updated to reflect this change, as is my fork of vscode-chrome-debug to use this API instead (link).

@roblourens roblourens merged commit 741f5fe into microsoft:master Sep 10, 2018
@roblourens
Copy link
Member

Thanks!

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.

2 participants