From 9e3a349fd6765afb4531192c21a5e7bc74c5d7a0 Mon Sep 17 00:00:00 2001 From: Claire Sarsam Date: Tue, 20 Jun 2017 16:30:28 -0700 Subject: [PATCH 1/2] Resolve ddc test failures --- lib/react_client.dart | 7 +- lib/src/ddc_emulated_function_name_bug.dart | 84 +++++++++++++++++++++ test/factory/common_factory_tests.dart | 6 +- test/lifecycle_test.dart | 6 +- 4 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 lib/src/ddc_emulated_function_name_bug.dart diff --git a/lib/react_client.dart b/lib/react_client.dart index afbed6f4..f66961e3 100644 --- a/lib/react_client.dart +++ b/lib/react_client.dart @@ -18,6 +18,7 @@ import "package:react/react_dom.dart"; import "package:react/react_dom_server.dart"; import "package:react/src/react_client/synthetic_event_wrappers.dart" as events; import 'package:react/src/typedefs.dart'; +import 'package:react/src/ddc_emulated_function_name_bug.dart' as ddc_emulated_function_name_bug; export 'package:react/react_client/react_interop.dart' show ReactElement, ReactJsComponentFactory; @@ -356,7 +357,11 @@ class ReactDomComponentFactoryProxy extends ReactComponentFactoryProxy { ReactDomComponentFactoryProxy(name) : this.name = name, - this.factory = React.createFactory(name); + this.factory = React.createFactory(name) { + if (ddc_emulated_function_name_bug.isBugPresent) { + ddc_emulated_function_name_bug.patchName(this); + } + } @override String get type => name; diff --git a/lib/src/ddc_emulated_function_name_bug.dart b/lib/src/ddc_emulated_function_name_bug.dart new file mode 100644 index 00000000..87f26e7e --- /dev/null +++ b/lib/src/ddc_emulated_function_name_bug.dart @@ -0,0 +1,84 @@ +/// Provides detection and patching of the bug described in , +/// in which getters/setters with the identifier `name` don't work for emulated function classes, like [UiProps]. +@JS() +library react.ddc_emulated_function_name_bug; + +import 'package:js/js.dart'; + +/// Create a reduced test case of the issue, using an emulated function pattern that is similar to [UiProps]. +/// +/// We can't use [UiProps] itself, since it uses [isBugPresent], and that would cause a cyclic initialization error. +class _NsmEmulatedFunctionWithNameProperty implements Function { + void call(); + + @override + noSuchMethod(i) {} + + String _name; + + // ignore: unnecessary_getters_setters + String get name => _name; + // ignore: unnecessary_getters_setters + set name(String value) => _name = value; +} + +/// Whether this bug, , is present in the current runtime. +/// +/// This performs functional detection of the bug, and will be `true` +/// only in the DDC and only in versions of the DDC where this bug is present. +final bool isBugPresent = (() { + const testValue = 'test value'; + + var testObject = new _NsmEmulatedFunctionWithNameProperty(); + + try { + // In the DDC, this throws: + // TypeError: Cannot assign to read only property 'name' of function 'function call(...args) { + // return call.call.apply(call, args); + // }' + testObject.name = testValue; + } catch(_) { + return true; + } + + try { + // We don't expect accessing this to throw, but just in case... + return testObject.name != testValue; + } catch(_) { + return true; + } +})(); + + +@JS() +@anonymous +class _PropertyDescriptor {} + +@JS('Object.getPrototypeOf') +external dynamic _getPrototypeOf(dynamic object); + +@JS('Object.getOwnPropertyDescriptor') +external _PropertyDescriptor _getOwnPropertyDescriptor(dynamic object, String propertyName); + +@JS('Object.defineProperty') +external void _defineProperty(dynamic object, String propertyName, _PropertyDescriptor descriptor); + +/// Patches the `name` property on the given [object] to have the expected behavior +/// by copying the property descriptor for `name` from the appropriate prototype. +/// +/// This is a noop if `name` is not a property on the given object. +/// +/// __This functionality is unstable, and should not be used when [isBugPresent] is `false`.__ +/// +/// This method also had undefined behavior on non-[UiProps] instances. +void patchName(dynamic object) { + var current = object; + while ((current = _getPrototypeOf(current)) != null) { + var nameDescriptor = _getOwnPropertyDescriptor(current, 'name'); + + if (nameDescriptor != null) { + _defineProperty(object, 'name', nameDescriptor); + return; + } + } +} diff --git a/test/factory/common_factory_tests.dart b/test/factory/common_factory_tests.dart index d3b4bcd2..ae2b7990 100644 --- a/test/factory/common_factory_tests.dart +++ b/test/factory/common_factory_tests.dart @@ -1,3 +1,5 @@ +@Tags(const ["ddcFailure"]) + import 'dart:js'; import 'package:test/test.dart'; @@ -33,7 +35,7 @@ void commonFactoryTests(Function factory) { test('multiple arguments', () { var instance = factory({}, 'one', 'two'); expect(getJsChildren(instance), equals(['one', 'two'])); - }); + }, tags: 'ddcFailure'); test('a List', () { var instance = factory({}, ['one', 'two',]); @@ -155,7 +157,7 @@ void _childKeyWarningTests(Function factory) { ); expect(consoleErrorCalled, isFalse, reason: 'should not have outputted a warning'); - }); + }, tags: 'ddcFailure'); test('when rendering custom Dart components', () { _renderWithUniqueOwnerName(() => diff --git a/test/lifecycle_test.dart b/test/lifecycle_test.dart index aef8faab..65396dc5 100644 --- a/test/lifecycle_test.dart +++ b/test/lifecycle_test.dart @@ -427,7 +427,7 @@ external List getUpdatingSetStateLifeCycleCalls(); @JS() external List getNonUpdatingSetStateLifeCycleCalls(); -ReactDartComponentFactoryProxy<_SetStateTest> SetStateTest = react.registerComponent(() => new _SetStateTest()) as ReactDartComponentFactoryProxy<_SetStateTest>; +ReactDartComponentFactoryProxy SetStateTest = react.registerComponent(() => new _SetStateTest()); class _SetStateTest extends react.Component { @override Map getDefaultProps() => {'shouldUpdate': true}; @@ -502,7 +502,7 @@ class _DefaultPropsCachingTest extends react.Component { render() => false; } -ReactDartComponentFactoryProxy<_DefaultPropsTest> DefaultPropsTest = react.registerComponent(() => new _DefaultPropsTest()) as ReactDartComponentFactoryProxy<_DefaultPropsTest>; +ReactDartComponentFactoryProxy DefaultPropsTest = react.registerComponent(() => new _DefaultPropsTest()); class _DefaultPropsTest extends react.Component { static int getDefaultPropsCallCount = 0; @@ -513,7 +513,7 @@ class _DefaultPropsTest extends react.Component { render() => false; } -ReactDartComponentFactoryProxy<_LifecycleTest> LifecycleTest = react.registerComponent(() => new _LifecycleTest()) as ReactDartComponentFactoryProxy<_LifecycleTest>; +ReactDartComponentFactoryProxy LifecycleTest = react.registerComponent(() => new _LifecycleTest()); class _LifecycleTest extends react.Component { List lifecycleCalls = []; From 97d11a166973a0727a1f52241591d8c3833d6e3f Mon Sep 17 00:00:00 2001 From: Claire Sarsam Date: Fri, 23 Jun 2017 09:25:42 -0700 Subject: [PATCH 2/2] Address CR feedback --- test/factory/common_factory_tests.dart | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/factory/common_factory_tests.dart b/test/factory/common_factory_tests.dart index ae2b7990..813758b0 100644 --- a/test/factory/common_factory_tests.dart +++ b/test/factory/common_factory_tests.dart @@ -1,5 +1,3 @@ -@Tags(const ["ddcFailure"]) - import 'dart:js'; import 'package:test/test.dart'; @@ -35,7 +33,7 @@ void commonFactoryTests(Function factory) { test('multiple arguments', () { var instance = factory({}, 'one', 'two'); expect(getJsChildren(instance), equals(['one', 'two'])); - }, tags: 'ddcFailure'); + }, tags: 'ddcFailure'); // This test cannot be run using ddc until https://github.com/dart-lang/sdk/issues/29904 is resolved test('a List', () { var instance = factory({}, ['one', 'two',]); @@ -157,7 +155,7 @@ void _childKeyWarningTests(Function factory) { ); expect(consoleErrorCalled, isFalse, reason: 'should not have outputted a warning'); - }, tags: 'ddcFailure'); + }, tags: 'ddcFailure'); // This test cannot be run using ddc until https://github.com/dart-lang/sdk/issues/29904 is resolved test('when rendering custom Dart components', () { _renderWithUniqueOwnerName(() =>