Skip to content

App Domain .callServiceExtension .restart .log #212

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

Merged
merged 13 commits into from
Mar 18, 2019
Merged

App Domain .callServiceExtension .restart .log #212

merged 13 commits into from
Mar 18, 2019

Conversation

grouma
Copy link
Member

@grouma grouma commented Mar 15, 2019

  • Add methods:
    • app.callServiceExtension
    • .apprestart
  • Add event:
    • app.log

Towards #191

@grouma grouma requested a review from jakemac53 March 15, 2019 21:32
Future<Map<String, dynamic>> _restart(Map<String, dynamic> args) async {
var appId = getStringArg(args, 'appId', required: true);
if (_appId != appId) throw ArgumentError.value(appId, 'appId', 'Not found');
// TODO(grouma) - Figure out what fullRestart means in this context.
Copy link
Member

Choose a reason for hiding this comment

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

Ah, full restart was the old name of hot restart. So, fullRestart==false is hot reload, and fullRestart==true is hot restart.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the details. I updated the code to reflect our current support.

// For now we will ignore.
// var fullRestart = getBoolArg(args, 'fullRestart') ?? false;
var pauseAfterRestart = getBoolArg(args, 'pause') ?? false;
if (pauseAfterRestart) {
Copy link
Member

@devoncarew devoncarew Mar 15, 2019

Choose a reason for hiding this comment

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

You may not want to throw here - we'd have to investigate. The IDEs will be sending it in when you start the app in a debug launch (as opposed to a 'run', w/o breakpoints enabled).

If the IDE does pass in pauseAfterRestart, it will wait until it gets a PausePostRequest event, remove all breakpoints, re-set all breakpoints, then resume the isolate. It does this to ensure that the new scripts, representing libraries that had been reloaded, have the correct breakpoints set.

Copy link
Member

Choose a reason for hiding this comment

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

Worth seeing what happens here first before doing a lot of work however.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted. I'll leave it as is and wait to hear back from @jwren and @DanTup.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can try this out Monday, but in VS Code we do always send pause when running in debug mode (to do what Devon describes above). The reason for this is that we need to re-send breakpoints after a hot restart (they got dropped) or hot reload (the code they were attached to may have been replaced with new code), and if we don't pause then we'll have race conditions (the code may have already run before we've sent the breakpoint).

@@ -43,6 +48,13 @@ class AppDomain extends Domain {
sendEvent('app.started', {
'appId': _appId,
});
_vmService.onStdoutEvent.listen((log) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this next to the streamListen?

webdev.stdin.add(utf8.encode('$extensionCall\n'));
// The example app sets up a service extension for printing.
await expectLater(
webdev.stdout, emitsThrough(startsWith('[{"event":"app.log"')));
Copy link
Contributor

Choose a reason for hiding this comment

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

This could easily pass for a different reason than the extension being called successfully (any log happening), I would check a bit more about the event if you can (check specifically for the expected print log line)

@grouma grouma merged commit c0690c5 into master Mar 18, 2019
@grouma grouma deleted the log-message branch March 18, 2019 19:45
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.

4 participants