Merge pull request #344 from Workiva/other-identityhashcode-throwing-case

FED-358 Work around identityHashCode throwing for non-extensible objects

Author: rmconsole6-wk

lib/react_client/js_interop_helpers.dart

@@ -10,19 +10,32 @@ import "package:js/js.dart";
 
 import 'js_backed_map.dart';
 
-/// Like [identityHashCode], but uses a different hash for JS objects to work around an issue where
-/// [identityHashCode] adds an unwanted `$identityHash` property on JS objects (https://github.com/dart-lang/sdk/issues/47595).
-int _jsObjectFriendlyIdentityHashCode(Object object) =>
-    object is JsMap ? _jsObjectHashCode(object) : identityHashCode(object);
+/// Like [identityHashCode], but uses a different hash for JS objects to work around:
+/// - an issue where [identityHashCode] adds an unwanted `$identityHash` property on JS objects: https://github.com/dart-lang/sdk/issues/47595
+/// - an issue where [identityHashCode] throws for frozen objects: https://github.com/dart-lang/sdk/issues/36354
+int _jsObjectFriendlyIdentityHashCode(Object object) {
+  // Try detecting JS objects so we don't add properties to them.
+  // Workaround for https://github.com/dart-lang/sdk/issues/47595
+  if (object is JsMap) return _anonymousJsObjectOrFrozenObjectHashCode(object);
 
-/// A hashCode implementation for JS objects.
+  // If this fails, then most likely the object is a frozen JS object, such as  props object or a variadic JSX children Array.
+  // Note that props objects are typically handled by the above is JsMap case, though.
+  // Fall back to a safe implementation.
+  try {
+    return identityHashCode(object);
+  } catch (_) {
+    return _anonymousJsObjectOrFrozenObjectHashCode(object);
+  }
+}
+
+/// A hashCode implementation for anonymous JS objects or frozen objects.
 ///
 /// Even though the current implementation of returning the same hash code for all values is low-quality
 /// since all JS objects will collide, it is valid since it always returns the same value for the same object.
 ///
-/// We also don't expect many JS objects to be passed into [jsifyAndAllowInterop], so the quality of this hash code
-/// is not of much concern.
-int _jsObjectHashCode(JsMap jsObject) => 0;
+/// We also don't expect many JS objects or frozen objects to be passed into [jsifyAndAllowInterop],
+/// so the quality of this hash code is not of much concern.
+int _anonymousJsObjectOrFrozenObjectHashCode(Object _) => 0;
 
 // The following code is adapted from `package:js` in the dart-lang/sdk repo:
 // https://github.com/dart-lang/sdk/blob/2.2.0/sdk/lib/js_util/dart2js/js_util_dart2js.dart#L27

test/react_client/js_interop_helpers_test.dart

@@ -50,6 +50,9 @@ class Foo {
   external num bar();
 }
 
+@JS()
+external dynamic createArray();
+
 main() {
   group('jsifyAndAllowInterop', () {
     test('converts a List', () {
@@ -131,5 +134,82 @@ main() {
       expect(objectKeys(convertedNestedJsObject), expectedProperties,
           reason: 'JS object should not have any additional properties');
     });
+
+    group('works as expected when the map contains frozen objects:', () {
+      test('anonymous object', () {
+        final frozenAnonymousObject = jsify({'foo': 'bar'});
+        expect(_getPrototypeOf(frozenAnonymousObject), _objectPrototype,
+            reason: 'test setup check; should not extend from another JS type');
+
+        _freeze(frozenAnonymousObject);
+        expect(_isFrozen(frozenAnonymousObject), isTrue, reason: 'test setup check; should have frozen');
+
+        dynamic jsObject;
+        expect(() {
+          jsObject = jsifyAndAllowInterop({
+            'frozenObject': frozenAnonymousObject,
+          });
+        }, returnsNormally, reason: 'should not throw when it encounters a frozen object');
+        final convertedNestedFrozenObject = getProperty(jsObject, 'frozenObject');
+        expect(convertedNestedFrozenObject, same(frozenAnonymousObject),
+            reason: 'JS object should have just gotten passed through');
+      });
+
+      test('non-anonymous object', () {
+        final frozenNonAnynymousObject = Foo(21);
+        expect(_getPrototypeOf(frozenNonAnynymousObject), isNot(_objectPrototype),
+            reason: 'test setup check; should extend from another JS type');
+
+        _freeze(frozenNonAnynymousObject);
+        expect(_isFrozen(frozenNonAnynymousObject), isTrue, reason: 'test setup check; should have frozen');
+
+        dynamic jsObject;
+        expect(() {
+          jsObject = jsifyAndAllowInterop({
+            'frozenObject': frozenNonAnynymousObject,
+          });
+        }, returnsNormally, reason: 'should not throw when it encounters a frozen object');
+        final convertedNestedFrozenObject = getProperty(jsObject, 'frozenObject');
+        expect(convertedNestedFrozenObject, same(frozenNonAnynymousObject),
+            reason: 'JS object should have just gotten passed through');
+      });
+
+      test('array object constructed in JS', () {
+        // Create this on the JS side so that we're not starting out with Dart's list implementation
+        // and potentially any wrapper classes.
+        final frozenArray = createArray();
+        expect(_getPrototypeOf(frozenArray), _arrayPrototype, reason: 'test setup check; should be an array');
+
+        _freeze(frozenArray);
+        expect(_isFrozen(frozenArray), isTrue, reason: 'test setup check; should have frozen');
+
+        dynamic jsObject;
+        expect(() {
+          jsObject = jsifyAndAllowInterop({
+            'frozenArray': frozenArray,
+          });
+        }, returnsNormally, reason: 'should not throw when it encounters a frozen object');
+        final convertedNestedFrozenObject = getProperty(jsObject, 'frozenArray');
+        // Note that we don't expect it to be `same(frozenArray)` since the array passes the `is Iterable` check,
+        // and thus gets processed in _convertDataTree.
+        expect(convertedNestedFrozenObject, equals(frozenArray),
+            reason: 'JS object should have been converted properly passed through');
+      });
+    });
   });
 }
+
+@JS('Object.prototype')
+external dynamic get _objectPrototype;
+
+@JS('Array.prototype')
+external dynamic get _arrayPrototype;
+
+@JS('Object.freeze')
+external void _freeze(Object object);
+
+@JS('Object.isFrozen')
+external bool _isFrozen(Object object);
+
+@JS('Object.getPrototypeOf')
+external dynamic _getPrototypeOf(Object object);

test/react_client/js_interop_helpers_test.html

@@ -20,6 +20,10 @@
           else
             return false;
         }
+
+        function createArray() {
+          return [1, 2, 3];
+        }
     </script>
 
     <link rel="x-dart-test" href="js_interop_helpers_test.dart">