Skip to content

[flutter web] Listen for service extension registration events to determine hot-restart method name#147897

Merged
auto-submit[bot] merged 2 commits into
flutter:masterfrom
elliette:web-hot-restart
May 7, 2024
Merged

[flutter web] Listen for service extension registration events to determine hot-restart method name#147897
auto-submit[bot] merged 2 commits into
flutter:masterfrom
elliette:web-hot-restart

Conversation

@elliette

@elliette elliette commented May 6, 2024

Copy link
Copy Markdown
Member

Some background:

  • Flutter registers the hot-restart service extension for all devices except web devices. For web devices, DWDS registers the service extensions.
  • When a user is debugging their code via VS Code, the VS Code Dart extension sends a hot-restart request via stdin/out to flutter_tools
  • flutter_tools then calls the "hotRestart" service extension (which, again, has been registered by DWDS)

Why is this change necessary?

In DWDS, we are changing how we register the "hotRestart" service extension (here is the PR for that: dart-lang/webdev#2388). Previously, we registered the "hotRestart" service extension on a client that was directly connected to the VmService. With dart-lang/webdev#2388, we will be registering the "hotRestart" service extension on a client that is connected to DDS.

When a service extension is registered against DDS, DDS adds a prefix to the service extension method name (e.g. "hotRestart" becomes "s0.hotRestart"). It informs clients of the service extension name via kServiceRegistered events sent to the Service stream.

Therefore, this change simply listens to those service extension registered events, and uses them to determine the "hotRestart" service extension's method name.

@flutter-dashboard

flutter-dashboard Bot commented May 6, 2024

Copy link
Copy Markdown

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@github-actions github-actions Bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 6, 2024
DanTup
DanTup approved these changes May 7, 2024
View reviewed changes

@DanTup DanTup left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The seems reasonable to me - we have to do the same in Dart-Code for calling services. I never understood why the names might be different.. but Jacob recently mentioned it might be because multiple clients can register the same service names, and this is how they'd be disambiguated. Dart-Code just calls the last one with that name, so if there are multiple they might not all be being called!

(I don't know the best way to add tests for this though, I'll defer to Chris for suggestions)

@christopherfujino

christopherfujino commented May 7, 2024

Copy link
Copy Markdown
Contributor

I was gonna say, "I'm assuming this is covered by existing tests", but then saw the test failures :) This LGTM once you update the test expectations:

00:05 +156 -18: test/general.shard/resident_web_runner_test.dart: cleanup of resources is safe to call multiple times [E]
  Expected: <Instance of 'Map<String, Object?>'> with `method`: 'streamListen' and `args`: {'streamId': 'Isolate'}
    Actual: {
              'jsonrpc': '2.0',
              'id': '53',
              'method': 'streamListen',
              'params': {'streamId': 'Service'}
            }

christopherfujino
christopherfujino approved these changes May 7, 2024
View reviewed changes

@christopherfujino christopherfujino left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@elliette elliette added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2024
@auto-submit auto-submit Bot merged commit b487b8c into flutter:master May 7, 2024
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
b487b8c [flutter web] Listen for service extension registration event…
11a6a10 
…s to determine hot-restart method name (flutter/flutter#147897)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
b487b8c [flutter web] Listen for service extension registration event…
c43f53c 
…s to determine hot-restart method name (flutter/flutter#147897)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
b487b8c [flutter web] Listen for service extension registration event…
9bc47ba 
…s to determine hot-restart method name (flutter/flutter#147897)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request May 8, 2024
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 8, 2024
@engine-flutter-autoroll
b487b8c [flutter web] Listen for service extension registration event…
c9635ff 
…s to determine hot-restart method name (flutter/flutter#147897)
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024
@engine-flutter-autoroll
b487b8c [flutter web] Listen for service extension registration event…
475fb8e 
…s to determine hot-restart method name (flutter/flutter#147897)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DanTup DanTup DanTup approved these changes
+1 more reviewer
@christopherfujino christopherfujino christopherfujino approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

autosubmit Merge PR when tree becomes green via auto submit App tool Affects the "flutter" command-line tool. See also t: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@elliette @christopherfujino @DanTup