Migrate devicelab to package:vm_service#71882
Migrate devicelab to package:vm_service#71882fluttergithubbot merged 5 commits intoflutter:masterfrom
Conversation
flutter-dashboard
bot
added
c: contributor-productivity
| registerExtension('ext.cocoonRunnerReady', | ||
| (String method, Map<String, String> parameters) async { | ||
| return ServiceExtensionResponse.result('"ready"'); | ||
| return ServiceExtensionResponse.result('{"result":"ready"}'); |
There was a problem hiding this comment.
The docs on this say that it has to be a valid "JSON object", which I assume means a {}. package:vm_service actually expects this to correspon to a Map object in Dart, whereas vm_service_client tolerated a String. A quoted string is valid JSON, but I guess not a valid JSON object.
/cc @bkonyi FYI.
| } | ||
|
|
||
| Future<VMIsolateRef> _connectToRunnerIsolate(Uri vmServiceUri) async { | ||
| class _ClientAndIsolate { |
There was a problem hiding this comment.
I don't love the name, but having a rough time coming up with something better. Maybe just Runner or RunnerClient? The client/isolate are sort of incidental just for the mechanics of the call, but conceptually this is a mechanism for firing off calls to the runner's service extension interface.
I sort of wonder if the naming issue is more of an abstraction issue -- this feels like a bit of an in-between abstraction like Pair. You could go all the way and name it Runner then rather than exposing callServiceExtention, expose the actual methods you care about runnerReady and runTask, basically a sort of cocoon service interface. Given that this is private API, up to you if you want to go that far in this patch, which is more about migration, but it's definitely worth adding a small doc comment for future visitors to this code.
There was a problem hiding this comment.
Changing it to RunnerClient and doing a quick refactor here.
This reverts commit 9d1d0cf.
3 participants
Description
Drop last references to package:vm_service_client from devicelab usage.
Related Issues
Fixes #37499
Tests
Update relevant tests. Will have to run through devicelab to be sure that this works, but it works locally on some spot checks.