Skip to content

Conversation

alan-knight
Copy link
Contributor

  • Split out a separate Debugger class
  • Made a couple of things public so they can be used from related libraries (like Debugger)
  • Added some stub tests

@alan-knight alan-knight requested a review from grouma May 7, 2019 23:56
_isolate = isolate;

_streamNotify(
streamNotify(
Copy link
Member

Choose a reason for hiding this comment

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

Not sure why this was changed to public. Likewise with scriptRefs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because Debugger needs to call them and it's in a separate library. We could instead make all the sub-components part files, but I'm not sure that's generally considered a good technique.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could move them to an internal library that could be used from both places, but that might require a bit more re-arrangement, e.g. moving _streamControllers to the other library as well.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Debugger isn't using them just yet but will in the near future. Keeping them public is probably preferable to a bunch of part files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw this is also why we only expose the VmServiceInterface now publicly instead of this class, so these don't leak outside the package (not without a src import)

_isolate = isolate;

_streamNotify(
streamNotify(
Copy link
Member

Choose a reason for hiding this comment

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

I see. Debugger isn't using them just yet but will in the near future. Keeping them public is probably preferable to a bunch of part files.

@alan-knight alan-knight merged commit 7d14c3b into master May 8, 2019
@alan-knight alan-knight deleted the cleanup branch May 8, 2019 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants