Skip to content

Daemon command boilerplate #200

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 2 commits into from
Mar 12, 2019
Merged

Daemon command boilerplate #200

merged 2 commits into from
Mar 12, 2019

Conversation

grouma
Copy link
Member

@grouma grouma commented Mar 12, 2019

WebDev will initially support Flutter oriented IDE plugins through the daemon protocol outlined here:
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/doc/daemon.md

Only a subsection of the protocol will be implemented. This command is a stop gap solution and is not intended to be supported long term. This code is heavily influenced by the logic in flutter_tools here:
https://github.com/flutter/flutter/blob/master/packages/flutter_tools/lib/src/commands/daemon.dart

@grouma grouma requested a review from jakemac53 March 12, 2019 20:02
class Daemon {
Daemon(
Stream<Map<String, dynamic>> commandStream,
this.sendCommand, {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider modeling sendCommand as a StreamSink instead of a function that takes commands

Copy link
Member Author

Choose a reason for hiding this comment

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

Gonna keep this as is since it aligns better with flutter_tools.

}

Future<void> shutdown(Map<String, dynamic> args) {
Timer.run(daemon.shutdown);
Copy link
Contributor

Choose a reason for hiding this comment

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

see if this can run right synchronously... my guess is its cruft from the flutter tools impl?

Copy link
Contributor

Choose a reason for hiding this comment

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

It does also seem a bit sketchy that this is the thing shutting down the daemon even though it isn't creating it... but that part is probably OK.

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to run it after we return the value. I'll add a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

could chain this off the future then with a cascade? Timer.run is a weird way to handle this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do in a follow up PR.

@grouma grouma merged commit 550fd19 into master Mar 12, 2019
@grouma grouma deleted the daemon-command branch March 12, 2019 21:47
@grouma grouma mentioned this pull request Mar 15, 2019
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.

2 participants