Merge pull request #989 from Workiva/clean-up-and-optimize-generated-props

FED-4215 Low-hanging dart2js code side optimizations

Author: btr-rmconsole-1[bot]

CHANGELOG.md

@@ -1,6 +1,7 @@
 # OverReact Changelog
 
 ## Unreleased
+- [#989] Optimize generated code to decrease dart2js compile size, saving ~577 bytes per component (when using `-03 --csp --minify`)
 - [#992] Fix compilation errors for legacy boilerplate defined in libraries with a Dart language version of >=3.0
 
 ## 5.4.6

example/boilerplate_versions/dart2_only/basic_component1.over_react.g.dart

@@ -60,10 +60,10 @@ abstract class ComponentGenerator extends BoilerplateDeclarationGenerator {
       // implemented for Component2.
       // This implementation here is necessary so that mixin accesses aren't compiled as index$ax
       outputContentsBuffer
-        ..writeln('  ${nullSafety ? 'late ' : ''}${propsNames.jsMapImplName} _cachedTypedProps;')
+        ..writeln('  ${nullSafety ? 'late ' : ''}${propsNames.implName} _cachedTypedProps;')
         ..writeln()
         ..writeln('  @override')
-        ..writeln('  ${propsNames.jsMapImplName} get props => _cachedTypedProps;')
+        ..writeln('  ${propsNames.implName} get props => _cachedTypedProps;')
         ..writeln()
         ..writeln('  @override')
         ..writeln('  set props(Map value) {')
@@ -81,8 +81,8 @@ abstract class ComponentGenerator extends BoilerplateDeclarationGenerator {
         ..writeln()
         ..writeln('  @override ')
         ..writeln(
-            '  ${propsNames.jsMapImplName} typedPropsFactoryJs(JsBackedMap${nullSafety ? '?' : ''} backingMap)'
-            ' => ${propsNames.jsMapImplName}(backingMap);')
+            '  ${propsNames.implName} typedPropsFactoryJs(JsBackedMap${nullSafety ? '?' : ''} backingMap)'
+            ' => ${propsNames.implName}(backingMap);')
         ..writeln();
     }
 
@@ -96,9 +96,9 @@ abstract class ComponentGenerator extends BoilerplateDeclarationGenerator {
       final stateNames = this.stateNames!;
       if (isComponent2) {
         outputContentsBuffer
-          ..writeln('  ${nullSafety ? 'late ' : ''}${stateNames.jsMapImplName} _cachedTypedState;')
+          ..writeln('  ${nullSafety ? 'late ' : ''}${stateNames.implName} _cachedTypedState;')
           ..writeln('  @override')
-          ..writeln('  ${stateNames.jsMapImplName} get state => _cachedTypedState;')
+          ..writeln('  ${stateNames.implName} get state => _cachedTypedState;')
           ..writeln()
           ..writeln('  @override')
           ..writeln('  set state(Map value) {')
@@ -111,8 +111,8 @@ abstract class ComponentGenerator extends BoilerplateDeclarationGenerator {
           ..writeln()
           ..writeln('  @override ')
           ..writeln(
-              '  ${stateNames.jsMapImplName} typedStateFactoryJs(JsBackedMap${nullSafety ? '?' : ''} backingMap)'
-              ' => ${stateNames.jsMapImplName}(backingMap);')
+              '  ${stateNames.implName} typedStateFactoryJs(JsBackedMap${nullSafety ? '?' : ''} backingMap)'
+              ' => ${stateNames.implName}(backingMap);')
           ..writeln();
       }
       outputContentsBuffer

example/boilerplate_versions/dart2_only/basic_component2.over_react.g.dart

@@ -143,30 +143,6 @@ class TypedMapNames {
   /// | Mixin-based | FooProps       | $FooProps    |
   String get implName => '$_prefix$privateSourcePrefix\$$_normalizedName';
 
-  /// The name of the generated concrete props/state implementation subclass
-  /// that can be backed by any Map.
-  ///
-  /// Example:
-  ///
-  /// | Version     | [consumerName] | Value                 |
-  /// |-------------|----------------|-----------------------|
-  /// | Legacy      | _$FooProps     | _$$FooProps$PlainMap  |
-  /// | Legacy      | _$_FooProps    | _$$_FooProps$PlainMap |
-  /// | Mixin-based | FooProps       | $FooProps$PlainMap    |
-  String get plainMapImplName => '$implName\$PlainMap';
-
-  /// The name of the generated concrete props/state implementation subclass
-  /// that can be backed only by JsBackedMaps.
-  ///
-  /// Example:
-  ///
-  /// | Version     | [consumerName] | Value              |
-  /// |-------------|----------------|--------------------|
-  /// | Legacy      | _$FooProps     | _$$FooProps$JsMap  |
-  /// | Legacy      | _$_FooProps    | _$$_FooProps$JsMap |
-  /// | Mixin-based | FooProps       | $FooProps$JsMap    |
-  String get jsMapImplName => '$implName\$JsMap';
-
   /// The name of the consumable props/state class.
   ///
   /// - For legacy backwards compatible boilerplate, this is the companion class.

lib/src/builder/codegen/component_generator.dart

@@ -95,18 +95,10 @@ abstract class TypedMapImplGenerator extends BoilerplateDeclarationGenerator {
   void _generateFactory() {
     assert(factoryNames.length == 1, 'factoryNames must have a length of 1');
 
-    outputContentsBuffer.write(
-        '${names.implName} ${factoryNames.first.implName}([Map${nullSafety ? '?' : ''} backingProps]) => ');
-
-    if (!isComponent2) {
-      /// _$$FooProps _$Foo([Map backingProps]) => _$$FooProps(backingProps);
-      outputContentsBuffer.writeln('${names.implName}(backingProps);');
-    } else {
-      /// _$$FooProps _$Foo([Map backingProps]) => backingProps == null ? $jsMapImplName(JsBackedMap()) : _$$FooProps(backingProps);
-      // Optimize this case for when backingProps is null to promote inlining of `jsMapImplName` typing
-      outputContentsBuffer.writeln(
-          'backingProps == null ? ${names.jsMapImplName}(JsBackedMap()) : ${names.implName}(backingProps);');
-    }
+    // _$$FooProps _$Foo([Map? backingProps]) => _$$FooProps(backingProps);
+    outputContentsBuffer.writeln(
+        '${names.implName} ${factoryNames.first.implName}([Map${nullSafety ? '?' : ''} backingProps])'
+        ' => ${names.implName}(backingProps);');
   }
 
   String _generateImplClassHeader();
@@ -168,70 +160,32 @@ abstract class TypedMapImplGenerator extends BoilerplateDeclarationGenerator {
             'componentFactoryName/propKeyNamespace must not be specified for state');
       }
     }
-
-    final classDeclaration = StringBuffer();
-    if (isComponent2) {
-      // This class will only have a factory constructor that instantiates one
-      // of two subclasses.
-      classDeclaration.write('abstract ');
-    }
-
-    classDeclaration
-      ..write(_generateImplClassHeader())
-      ..write(' {');
-
     final propsOrState = isProps ? 'props' : 'state';
 
-    // Class declaration
     final buffer = StringBuffer()
+      // Class declaration
       ..writeln('// Concrete $propsOrState implementation.')
       ..writeln('//')
       ..writeln(
           '// Implements constructor and backing map${isProps ? ', and links up to generated component factory' : ''}.')
       ..write(internalGeneratedMemberDeprecationLine())
-      ..writeln(classDeclaration);
-
-    // Constructors
-    if (isComponent2) {
-      buffer
-        ..writeln('  ${names.implName}._();')
-        ..writeln()
-        ..writeln('  factory ${names.implName}(Map${nullSafety ? '?' : ''} backingMap) {')
-        ..writeln('    if (backingMap == null || backingMap is JsBackedMap) {')
-        ..writeln(
-            '      return ${names.jsMapImplName}(backingMap as JsBackedMap${nullSafety ? '?' : ''});')
-        ..writeln('    } else {')
-        ..writeln('      return ${names.plainMapImplName}(backingMap);')
-        ..writeln('    }')
-        ..writeln('  }');
-    } else {
-      buffer
-        ..writeln(
-            '  // This initializer of `_$propsOrState` to an empty map, as well as the reassignment')
-        ..writeln(
-            '  // of `_$propsOrState` in the constructor body is necessary to work around a DDC bug: https://github.com/dart-lang/sdk/issues/36217')
-        // TODO need to remove this workaround once https://github.com/dart-lang/sdk/issues/36217 is fixed get nice dart2js output
-        ..writeln(
-            '  ${names.implName}(Map${nullSafety ? '?' : ''} backingMap) : this._$propsOrState = {} {')
-        ..writeln('     this._$propsOrState = backingMap ?? {};')
-        ..writeln('  }');
-    }
+      ..write(_generateImplClassHeader())
+      ..writeln(' {')
+      // Constructor
+      ..writeln('  ${names.implName}([Map${nullSafety ? '?' : ''} backingMap])'
+          ' : this.$propsOrState = backingMap ?? JsBackedMap();');
 
     // This needs to be a top-level member and not a static member, and it needs to be unique
     // to avoid collisions across typed map impls within the library, potentially in multiple parts.
     // So, we'll just namespace it by the impl name.
     final topLevelGetPropKeyAliasName = '_\$getPropKey\$${names.implName}';
 
     // Members
-    if (!isComponent2) {
-      buffer
-        ..writeln()
-        ..writeln('  /// The backing $propsOrState map proxied by this class.')
-        ..writeln('  @override')
-        ..writeln('  Map get $propsOrState => _$propsOrState;')
-        ..writeln('  Map _$propsOrState;');
-    }
     buffer
+      ..writeln()
+      ..writeln('  /// The backing $propsOrState map proxied by this class.')
+      ..writeln('  @override')
+      ..writeln('  final Map $propsOrState;')
       ..writeln()
       ..writeln(
           '  /// Let `${isProps ? 'UiProps' : 'UiState'}` internals know that this class has been generated.')
@@ -296,38 +250,6 @@ abstract class TypedMapImplGenerator extends BoilerplateDeclarationGenerator {
         ..writeln('const $topLevelGetPropKeyAliasName = getPropKey;');
     }
 
-    // Component2-specific classes
-    if (isComponent2) {
-      // TODO need to remove this workaround once https://github.com/dart-lang/sdk/issues/36217 is fixed get nice dart2js output
-      buffer
-        ..writeln()
-        ..writeln('''
-// Concrete $propsOrState implementation that can be backed by any [Map].
-${internalGeneratedMemberDeprecationLine()}class ${names.plainMapImplName}$typeParamsOnClass extends ${names.implName}$typeParamsOnSuper {
-  // This initializer of `_$propsOrState` to an empty map, as well as the reassignment
-  // of `_$propsOrState` in the constructor body is necessary to work around a DDC bug: https://github.com/dart-lang/sdk/issues/36217
-  ${names.plainMapImplName}(Map${nullSafety ? '?' : ''} backingMap) : this._$propsOrState = {}, super._() {
-     this._$propsOrState = backingMap ?? {};
-  }
-  /// The backing $propsOrState map proxied by this class.
-  @override
-  Map get $propsOrState => _$propsOrState;
-  Map _$propsOrState;
-}
-// Concrete $propsOrState implementation that can only be backed by [JsMap],
-// allowing dart2js to compile more optimal code for key-value pair reads/writes.
-${internalGeneratedMemberDeprecationLine()}class ${names.jsMapImplName}$typeParamsOnClass extends ${names.implName}$typeParamsOnSuper {
-  // This initializer of `_$propsOrState` to an empty map, as well as the reassignment
-  // of `_$propsOrState` in the constructor body is necessary to work around a DDC bug: https://github.com/dart-lang/sdk/issues/36217
-  ${names.jsMapImplName}(JsBackedMap${nullSafety ? '?' : ''} backingMap) : this._$propsOrState = JsBackedMap(), super._() {
-     this._$propsOrState = backingMap ?? JsBackedMap();
-  }
-  /// The backing $propsOrState map proxied by this class.
-  @override
-  JsBackedMap get $propsOrState => _$propsOrState;
-  JsBackedMap _$propsOrState;
-}''');
-    }
     return buffer.toString();
   }
 }
@@ -488,7 +410,7 @@ class _TypedMapImplGenerator extends TypedMapImplGenerator {
         '${factoryName.privateConfigName} = UiFactoryConfig(\n'
         'propsFactory: PropsFactory(\n'
         'map: (map) => ${names.implName}(map),\n'
-        'jsMap: (map) => ${names.jsMapImplName}(map),),\n'
+        'jsMap: (map) => ${names.implName}(map),),\n'
         'displayName: \'${factoryName.consumerName}\');\n\n'
         '@Deprecated(r\'Use the private variable, ${factoryName.privateConfigName}, instead \'\n'
         '\'and update the `over_react` lower bound to version 4.1.0. \'\n'