chore(android): optimize proxy constructor lookup (#12774)

Fixes TIMOB-28437
tidev · Sep 10, 2021 · 54132d2 · 54132d2
1 parent 2c9488d
commit 54132d2
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 11 deletions.
diff --git a/...apt/src/main/resources/org/appcelerator/kroll/annotations/generator/ProxyBindingV8.cpp.fm b/...apt/src/main/resources/org/appcelerator/kroll/annotations/generator/ProxyBindingV8.cpp.fm
@@ -70,6 +70,7 @@ void ${className}::bindProxy(Local<Object> exports, Local<Context> context)
 	moduleInstance.Reset(isolate, instance);
 	<#else>
 	exports->Set(context, nameSymbol, constructor);
+	exports->Set(context, constructorSymbol.Get(isolate), constructor);
 	</#if>
 }
 

diff --git a/android/runtime/v8/src/native/ProxyFactory.cpp b/android/runtime/v8/src/native/ProxyFactory.cpp
@@ -54,23 +54,25 @@ Local<Object> ProxyFactory::createV8Proxy(v8::Isolate* isolate, Local<Value> cla
 		return Local<Object>();
 	}
 
-	// FIXME: We pick the first item in exports as the constructor. We should do something more intelligent (for ES6 look at default export?)
-	Local<Array> names;
-	MaybeLocal<Array> possibleNames = exports->GetPropertyNames(context);
-	if (!possibleNames.ToLocal(&names) || names->Length() < 1) {
-		v8::String::Utf8Value classStr(isolate, className);
-		LOGE(TAG, "Failed to find constructor in exports for %s", *classStr);
-		LOG_JNIENV_ERROR("while creating V8 Proxy.");
-		return Local<Object>();
+
+	bool hasConstructor = exports->HasOwnProperty(context, Proxy::constructorSymbol.Get(isolate)).FromMaybe(false);
+	MaybeLocal<Value> possibleConstructor;
+	if (hasConstructor) {
+		possibleConstructor = exports->Get(context, Proxy::constructorSymbol.Get(isolate));
+	} else {
+		// Fallback to old behaviour of selecting first property for constructor.
+		Local<Array> propertyNames;
+		MaybeLocal<Array> maybePropertyNames = exports->GetPropertyNames(context);
+		if (maybePropertyNames.ToLocal(&propertyNames) && propertyNames->Length() > 0) {
+			possibleConstructor = exports->Get(context, propertyNames->Get(context, 0).ToLocalChecked());
+		}
 	}
-	MaybeLocal<Value> possibleConstructor = exports->Get(context, names->Get(context, 0).ToLocalChecked());
-	if (possibleConstructor.IsEmpty()) {
+	if (possibleConstructor.IsEmpty() || !possibleConstructor.ToLocalChecked()->IsFunction()) {
 		v8::String::Utf8Value classStr(isolate, className);
 		LOGE(TAG, "Failed to get constructor in exports for %s", *classStr);
 		LOG_JNIENV_ERROR("while creating V8 Proxy.");
 		return Local<Object>();
 	}
-
 	Local<Function> creator = possibleConstructor.ToLocalChecked().As<Function>();
 
 	Local<Value> javaObjectExternal = External::New(isolate, javaProxy);

diff --git a/tests/Resources/core.runtime.test.js b/tests/Resources/core.runtime.test.js
@@ -33,6 +33,14 @@ describe.windowsBroken('Core', () => {
 			});
 
 			describe('Proxy', () => {
+				describe('constructor', () => {
+					it('should be correctly set on a proxy', () => {
+						const view = Ti.UI.createView();
+						view.should.have.property('constructor');
+						should(view.constructor).be.a.Function();
+					});
+				});
+
 				describe('Properties', () => {
 					it('should check for properties on the object\'s target', () => {
 						const view = Ti.UI.createView({