Aggressively seal classes in `package:devtools_app_shared` #6235

kenzieschmoll · 2023-08-22T20:08:08Z

devtools_app_shared will be published on pub and will be used by DevTools extension authors to build extensions. In order to limit breaking changes to DevTools extensions, we would like to "seal" as many classes in this shared library as possible.

This PR aggressively marks classes as final, where possible. In cases where marking classes final would prohibit us from being able to effectively write tests, we use the @sealed annotation from package:meta.

@bkonyi @jacob314

bkonyi · 2023-08-22T20:24:10Z

It feels weird using @sealed and final in the same file, but I think this is the right thing to do for testability.

I think we should be very aggressive with marking anything we expose through this package as final or some variation of final. We can always relax restrictions if we have a reason to, but once we release we can't tighten them.

packages/devtools_app/test/shared/instance_viewer_test.dart

packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart

kenzieschmoll · 2023-08-22T21:58:31Z

packages/devtools_app/lib/src/service/service_manager.dart

@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.

+import 'dart:async';
+


this file should be reviewed

kenzieschmoll · 2023-08-22T22:01:49Z

packages/devtools_test/lib/src/mocks/generated_mocks_factories.dart

@@ -161,6 +161,13 @@ MockVmServiceWrapper createMockVmServiceWrapperWithDefaults() {
  return service;
 }

+MockServiceConnectionManager createMockServiceConnectionWithDefaults() {


kenzieschmoll · 2023-08-22T22:02:07Z

packages/devtools_test/lib/src/mocks/fake_service_manager.dart

@@ -20,35 +20,103 @@ import 'fake_vm_service_wrapper.dart';
 import 'generated.mocks.dart';
 import 'mocks.dart';

-class FakeServiceManager extends Fake implements ServiceConnectionManager {
-  FakeServiceManager({
+class FakeServiceConnectionManager extends Fake


this file should be reviewed

kenzieschmoll · 2023-08-22T22:03:09Z

packages/devtools_app_shared/lib/src/service/service_manager.dart

@@ -7,6 +7,7 @@ import 'dart:core';

 import 'package:flutter/foundation.dart';
 import 'package:logging/logging.dart';
+import 'package:meta/meta.dart';


this file should be reviewed

kenzieschmoll · 2023-08-22T22:03:42Z

packages/devtools_app_shared/lib/src/service/connected_app.dart

@@ -188,3 +193,40 @@ class ConnectedApp {
          flutterVersionKey: flutterVersionNow!.version,
      };
 }
+
+final class OfflineConnectedApp extends ConnectedApp {


moved from package:devtools_app and marked final

jacob314 · 2023-08-22T22:52:53Z

packages/devtools_app/lib/src/service/service_manager.dart

@@ -150,7 +196,8 @@ class ServiceConnectionManager extends ServiceManager<VmServiceWrapper> {
  ///
  /// Throws an Exception if no 'FlutterView' is present in this isolate.
  Future<String> get flutterViewId async {


superficially extensions like this seem equally useful for devtools extensions as for devtools proper.

Agreed we may want to expose this. For now, I'd like to keep the API small and add more to devtools_app_shared as it makes sense.

jacob314 · 2023-08-22T22:54:18Z

packages/devtools_app/lib/src/service/service_manager.dart

-  @override
-  List<IsolateRef> isolatesFromVm(VM? vm) {
-    return vm?.isolatesForDevToolsMode() ?? <IsolateRef>[];
+  Future<String?> rootLibraryForMainIsolate() async {


this seems like something an extension might want to know. how do you draw the line on whether to include it in the shared code or not?

Agreed we may want to expose this. For now, I'd like to keep the API small and add more to devtools_app_shared as it makes sense. At this point I am exposing everything needed for the package:provider extension as a customer zero, and will expose more as we get a better idea of what more initial customers need.

jacob314 · 2023-08-22T23:00:53Z

It feels weird using @sealed and final in the same file, but I think this is the right thing to do for testability.

I think we should be very aggressive with marking anything we expose through this package as final or some variation of final. We can always relax restrictions if we have a reason to, but once we release we can't tighten them.

Turns out there is an alternative to using @sealed that still allows reopening classes for testing purposes only with only a couple lines of boilerplate.
dart-lang/language#3106 (comment)
Given that, I believe we should deprecate @sealed to avoid confusion and point users to this technique.

bkonyi

LGTM, but we should probably get another +1 as I had a hard time reviewing this thoroughly (Chrome + GitHub are thrashing hard when I open the "Files changed" tab).

bkonyi · 2023-08-24T13:55:40Z

packages/devtools_app/lib/src/service/service_manager.dart

@@ -33,7 +35,35 @@ const debugLogServiceProtocolEvents = false;

 const defaultRefreshRate = 60.0;

-class ServiceConnectionManager extends ServiceManager<VmServiceWrapper> {
+class ServiceConnectionManager {
+  ServiceConnectionManager() {


Nit: can't this be ServiceConnectionManager() : serviceManager = ServiceManager().. //...? Then I think you can remove the late from the declaration of serviceManager as well.

the callbacks can't be accessed in an initializer.

bkonyi · 2023-08-24T14:12:16Z

Also, do we plan on using the solution from dart-lang/language#3106 (comment) or landing with @sealed for now?

kenzieschmoll · 2023-08-24T15:35:41Z

Also, do we plan on using the solution from dart-lang/language#3106 (comment) or landing with @sealed for now?

In this comment here, I explain why we need to use @sealed right now for classes we need to mock and not fake. I also filed #6239 to track removing @sealed if we come up with a better solution or decide to rewrite our tests to not use mocks for these "sealed" classes.

CoderDake

Seems alright to me, but it was definitely a lot to review :P

CoderDake

oops I meant to approve

Aggressively seal classes in package:devtools_app_shared

a2eb0cd

jacob314 reviewed Aug 22, 2023

View reviewed changes

packages/devtools_app/test/shared/instance_viewer_test.dart Show resolved Hide resolved

jacob314 reviewed Aug 22, 2023

View reviewed changes

packages/devtools_app_shared/lib/src/service/eval_on_dart_library.dart Show resolved Hide resolved

kenzieschmoll added 2 commits August 22, 2023 14:51

more final

b43f55b

refactors

34b54e9

kenzieschmoll commented Aug 22, 2023

View reviewed changes

kenzieschmoll added 3 commits August 22, 2023 15:04

fixes

0c51188

formatting

fd0f11b

formatting

d48f6f3

jacob314 reviewed Aug 22, 2023

View reviewed changes

fix fakes and mocks

e321148

kenzieschmoll marked this pull request as ready for review August 23, 2023 21:46

kenzieschmoll requested review from bkonyi, elliette, polina-c, CoderDake and a team as code owners August 23, 2023 21:46

kenzieschmoll requested review from bkonyi and removed request for bkonyi August 23, 2023 21:47

kenzieschmoll added run-dcm-workflow release-notes-not-required labels Aug 23, 2023

kenzieschmoll mentioned this pull request Aug 23, 2023

Create and publish package:devtools_app_shared #6208

Closed

5 tasks

Merge branch 'master' of github.com:flutter/devtools into example-dtas

d67bdb8

provide dummy

457d0d7

bkonyi approved these changes Aug 24, 2023

View reviewed changes

CoderDake suggested changes Aug 24, 2023

View reviewed changes

CoderDake approved these changes Aug 24, 2023

View reviewed changes

kenzieschmoll merged commit 7ff0097 into flutter:master Aug 24, 2023
15 checks passed

kenzieschmoll deleted the example-dtas branch August 24, 2023 16:02

polina-c mentioned this pull request Aug 29, 2023

Releasing from 2.27.0-dev.17 to 2.27.0 polina-c/devtools#75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aggressively seal classes in `package:devtools_app_shared` #6235

Aggressively seal classes in `package:devtools_app_shared` #6235

kenzieschmoll commented Aug 22, 2023 •

edited

Loading

bkonyi commented Aug 22, 2023

kenzieschmoll Aug 22, 2023

kenzieschmoll Aug 22, 2023

kenzieschmoll Aug 22, 2023

kenzieschmoll Aug 22, 2023

kenzieschmoll Aug 22, 2023

jacob314 Aug 22, 2023

kenzieschmoll Aug 23, 2023

jacob314 Aug 22, 2023

kenzieschmoll Aug 23, 2023

jacob314 commented Aug 22, 2023

bkonyi left a comment

bkonyi Aug 24, 2023

kenzieschmoll Aug 24, 2023

bkonyi commented Aug 24, 2023

kenzieschmoll commented Aug 24, 2023

CoderDake left a comment

CoderDake left a comment

Aggressively seal classes in package:devtools_app_shared #6235

Aggressively seal classes in package:devtools_app_shared #6235

Conversation

kenzieschmoll commented Aug 22, 2023 • edited Loading

bkonyi commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacob314 commented Aug 22, 2023

bkonyi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkonyi commented Aug 24, 2023

kenzieschmoll commented Aug 24, 2023

CoderDake left a comment

Choose a reason for hiding this comment

CoderDake left a comment

Choose a reason for hiding this comment

Aggressively seal classes in `package:devtools_app_shared` #6235

Aggressively seal classes in `package:devtools_app_shared` #6235

kenzieschmoll commented Aug 22, 2023 •

edited

Loading