Provide static checking by refactoring how the service extensions used by layout_explorer are defined. by jacob314 · Pull Request #1682 · flutter/devtools

jacob314 · 2020-02-27T21:46:52Z

Previously these extensions were defined as string literals which eliminated static checking.
They are now defined as a slightly magical dart file under assets/scripts
which gives you full dart code completion and analysis server error checking.
With this change, writing these polyfills is simple enough that we should
write all new widget_inspector.dart functionality here rather than in
package:flutter/src/widgets/widget_inspector.dart only moving it to
widget_inspector.dart once we are confident we will use the feature for the long term.

jacob314 · 2020-02-27T21:52:36Z

+
+void addServiceExtensions() {
+  // INSPECTOR_POLYFILL_SCRIPT_START
+  T toEnumEntry<T>(List<T> enumEntries, String name) {


None of this code is new. It is just moved from the existing code snippets within source code that @adalberht already defined.

jacob314

Fyi @adalberht your existing extensions were easy to refactor this way so that I could get better static checking.

jacob314 · 2020-02-27T21:53:46Z

+    return Future.value(<String, Object>{'result': succeed});
+  }
+
+  void registerHelper(String name, ServiceExtensionCallback callback) {


except for this method.. it is new.

kenzieschmoll · 2020-02-27T23:00:03Z

+// features should be exposed with a wide range of Flutter versions to get
+// useful user feedback.
+//
+// We current execute this library using eval but that should be viewed as an


nit: r/current/currently

kenzieschmoll · 2020-02-27T23:04:07Z

+      // It is not a fatal error if some of the extensions fail to register
+      // as could be the case if some are already defined directly within
+      // package:flutter for the version of package:flutter being used.
+      print('Warning: unable to register service extension \'$name\'');


can you use the DevTools logger here instead of print?

you can't use it directly as this code is executed on the users device. However I have now plumbed it through so errors are tracked and logged back in DevTools. Did a little bit of other refactoring to make it happen.

albertusdev · 2020-02-28T07:13:20Z

This is so cool 🥺

kenzieschmoll · 2020-03-03T16:54:52Z

albertusdev · 2020-02-28T11:46:10Z

 class RegistrableServiceExtension {
  RegistrableServiceExtension({
    @required this.name,
-    @required this.statements,
-    this.requireEnumDeserialization = false,
  });

  final String name;

-  /// Statements inside the callback.
-  /// Should end with ';'
-  /// To deserialize the required parameters, use variable 'parameters'.
-  /// See [getLayoutExplorerNode] for example.
-  final String statements;
-
-  // Whether this callback need to deserialize enum values or not.
-  final bool requireEnumDeserialization;
-
-  // Generated ServiceExtensionCallback definition
-  String get callbackDefinition {
-    return '''
-      ${requireEnumDeserialization ? toEnumEntryCodeDefinition : ''}
-      Future<Map<String, dynamic>> $name(Map<String, String> parameters){
-        $statements
-      }
-    ''';
-  }
-
-  static final getLayoutExplorerNode = RegistrableServiceExtension(
-    name: 'getLayoutExplorerNode',
-    statements: '''
-        final String id = parameters['id'];
-        final int subtreeDepth = int.parse(parameters['subtreeDepth']);
-        final String groupName = parameters['groupName'];
-        Map<String, Object> result = {};
-        final instance = WidgetInspectorService.instance;
-        final root = instance.toObject(id);
-        if (root == null) {
-          result = null;
-        } else {
-          result = instance._nodeToJson(
-            root,
-            InspectorSerializationDelegate(
-              groupName: groupName,
-              summaryTree: true,
-              subtreeDepth: subtreeDepth,
-              includeProperties: false,
-              service: instance,
-              addAdditionalPropertiesCallback: (node, delegate) {
-                final Map<String, Object> additionalJson = <String, Object>{};
-                final Object value = node.value;
-                if (value is Element) {
-                  final renderObject = value.renderObject;
-                  additionalJson['renderObject'] =
-                      renderObject.toDiagnosticsNode()?.toJsonMap(
-                        delegate.copyWith(
-                          subtreeDepth: 0,
-                          includeProperties: true,
-                        ),
-                      );
-                  final Constraints constraints = renderObject.constraints;
-                  if (constraints != null) {
-                    final Map<String, Object> constraintsProperty = <
-                        String,
-                        Object>{
-                      'type': constraints.runtimeType.toString(),
-                      'description': constraints.toString(),
-                    };
-                    if (constraints is BoxConstraints) {
-                      constraintsProperty.addAll(<String, Object>{
-                        'minWidth': constraints.minWidth.toString(),
-                        'minHeight': constraints.minHeight.toString(),
-                        'maxWidth': constraints.maxWidth.toString(),
-                        'maxHeight': constraints.maxHeight.toString(),
-                      });
-                    }
-                    additionalJson['constraints'] = constraintsProperty;
-                  }
-                  if (renderObject is RenderBox) {
-                    additionalJson['size'] = <String, Object>{
-                      'width': renderObject.size.width.toString(),
-                      'height': renderObject.size.height.toString(),
-                    };
-
-                    final ParentData parentData = renderObject.parentData;
-                    if (parentData is FlexParentData) {
-                      additionalJson['flexFactor'] = parentData.flex;
-                      additionalJson['flexFit'] =
-                          describeEnum(parentData.fit ?? FlexFit.tight);
-                    }
-                  }
-                }
-                return additionalJson;
-              }
-            ),
-          );
-        }
-        return Future<Map<String, Object>>.value(<String, Object>{
-          'result': result,
-        });
-      ''',
-  );
-
-  static final setFlexFit = RegistrableServiceExtension(
-    name: 'setFlexFit',
-    statements: '''
-      final String id = parameters['id'];
-      final FlexFit flexFit =
-          toEnumEntry<FlexFit>(FlexFit.values, parameters['flexFit']);
-      dynamic object = WidgetInspectorService.instance.toObject(id);
-      if (object == null) return null;
-      final render = object.renderObject;
-      final parentData = render.parentData;
-      bool succeed = false;
-      if (parentData is FlexParentData) {
-        parentData.fit = flexFit;
-        render.markNeedsLayout();
-        succeed = true;
-      }
-      return Future<Map<String, Object>>.value(<String, Object>{
-        'result': succeed,
-      });
-    ''',
-    requireEnumDeserialization: true,
-  );
-  static final setFlexFactor = RegistrableServiceExtension(
-    name: 'setFlexFactor',
-    statements: '''
-      final String id = parameters['id'];
-      final String flexFactor = parameters['flexFactor'];
-      final int factor = flexFactor == "null" ? null : int.parse(flexFactor);
-      final dynamic object = WidgetInspectorService.instance.toObject(id);
-      if (object == null) return null;
-      final render = object.renderObject;
-      final parentData = render.parentData;
-      bool succeed = false;
-      if (parentData is FlexParentData) {
-        parentData.flex = factor;
-        render.markNeedsLayout();
-        succeed = true;
-      }
-      return Future<Map<String, Object>>.value(<String, Object>{
-        'result': succeed,
-      });
-    ''',
-  );
-  static final setFlexProperties = RegistrableServiceExtension(
-    name: 'setFlexProperties',
-    statements: '''
-      final String id = parameters['id'];
-      final MainAxisAlignment mainAxisAlignment = toEnumEntry<MainAxisAlignment>(
-        MainAxisAlignment.values,
-        parameters['mainAxisAlignment'],
-      );
-      final CrossAxisAlignment crossAxisAlignment = toEnumEntry<CrossAxisAlignment>(
-        CrossAxisAlignment.values,
-        parameters['crossAxisAlignment'],
-      );
-      final dynamic object = WidgetInspectorService.instance.toObject(id);
-      if (object == null) return null;
-      final render = object.renderObject;
-      bool succeed = false;
-      if (render is RenderFlex) {
-        render.mainAxisAlignment = mainAxisAlignment;
-        render.crossAxisAlignment = crossAxisAlignment;
-        render.markNeedsLayout();
-        succeed = true;
-      }
-      return Future<Map<String, Object>>.value(<String, Object>{
-        'result': succeed,
-      });
-    ''',
-    requireEnumDeserialization: true,
-  );
+  static final getLayoutExplorerNode =
+      RegistrableServiceExtension(name: 'getLayoutExplorerNode');
+  static final setFlexFit = RegistrableServiceExtension(name: 'setFlexFit');
+  static final setFlexFactor =
+      RegistrableServiceExtension(name: 'setFlexFactor');
+  static final setFlexProperties =
+      RegistrableServiceExtension(name: 'setFlexProperties');


I think this class can be removed now and be replaced with pure string

I'm leaving it for now because I expect we'll add some more logic back to it at some point tracking minimum versions required or similar.

… for static checking. Previously these extensions were defined as string literals which eliminated static checking. They are now defined as a slightly magical dart file under assets/scripts which gives you full dart code completion and analysis server error checking. With this change, writing these polyfills is simple enough that we should write all new widget_inspector.dart functionality here rather than in package:flutter/src/widgets/widget_inspector.dart only moving it to widget_inspector.dart once we are confident we will use the feature for the long term.

jacob314 · 2020-03-12T19:48:13Z

ptal

kenzieschmoll · 2020-03-12T20:23:50Z

  registerHelper('setFlexFit', setFlexFit);
  registerHelper('setFlexFactor', setFlexFactor);
  registerHelper('setFlexProperties', setFlexProperties);
+  return failures.isNotEmpty


why is this returning at all when the method signature return is void

it isn't void. it is String addServiceExtensions() {

oh whoops, missed a bracket.

kenzieschmoll · 2020-03-12T20:26:41Z

+      return stringRef.valueAsString;
+
+    final dynamic result = await getObject(isolateId, stringRef.id,
+        offset: 0, count: stringRef.length);


trailing comma

kenzieschmoll · 2020-03-12T20:27:26Z

+    String onUnavailable(String truncatedValue),
+  }) async {
+    if (stringRef == null) return null;
+    if (stringRef.valueAsStringIsTruncated != true)


you can just write !stringRef.valueAsStringIsTruncated

valueAsStringIsTruncated looks like it may be null. would be nice to have null safety so this didn't matter.
expect when we get null safety support there will be a lint for this case.
for now I'd rather just keep this consistent with what this method looked like back before I moved it to a central location.

kenzieschmoll · 2020-03-12T20:27:46Z

+    if (stringRef.valueAsStringIsTruncated != true)
+      return stringRef.valueAsString;
+
+    final dynamic result = await getObject(isolateId, stringRef.id,


do you have to specify dynamic here for type checking to work below?

this method is just being moved to a central location.
however I agree with the cleanup. removed the dynamic and used type inference.

kenzieschmoll · 2020-03-12T20:28:07Z

+    final dynamic result = await getObject(isolateId, stringRef.id,
+        offset: 0, count: stringRef.length);
+    if (result is Instance) {
+      final Instance obj = result;


is this type necessary? Does the type promotion not happen on its own?

yep. this code was written a long time ago.

kenzieschmoll · 2020-03-12T20:50:30Z

+    String onUnavailable(String truncatedValue),
+  }) async {
+    if (stringRef == null) return null;
+    if (stringRef.valueAsStringIsTruncated != true)


jacob314 requested a review from kenzieschmoll February 27, 2020 21:46

jacob314 commented Feb 27, 2020

View reviewed changes

jacob314 mentioned this pull request Feb 27, 2020

layout explorer needs to surface exceptions triggered by disallowed flex layouts #1650

Open

kenzieschmoll approved these changes Feb 27, 2020

View reviewed changes

albertusdev reviewed Mar 5, 2020

View reviewed changes

jacob314 and others added 2 commits March 12, 2020 09:12

code review comment fixes.

3c9d2f8

jacob314 force-pushed the polyfill branch from 86ec759 to 3c9d2f8 Compare March 12, 2020 19:45

kenzieschmoll reviewed Mar 12, 2020

View reviewed changes

Code review comment fixes

f17de18

kenzieschmoll approved these changes Mar 12, 2020

View reviewed changes

jacob314 merged commit ab727ae into flutter:master Mar 12, 2020

jacob314 deleted the polyfill branch March 12, 2020 21:41

Conversation

jacob314 commented Feb 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertusdev commented Feb 28, 2020

Uh oh!

kenzieschmoll commented Mar 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants