Skip to content

Commit

Permalink
[TIMOB-26119] TiAPI: Deprecate getter/setter accessor methods for pro…
Browse files Browse the repository at this point in the history
…perties:

* Add deprecation warnings on iOS for getX()/setX() accessor methods wrapping properties.
* Add deprecation notices for get/set methods that just delegate for properties on Android
* Fix so Android doesn't expose a Ti.Filesystem.File.directoryListing property improperly
* Spit out deprecation warnings for typical getter/setter methods wrapping a property on Android
* Default to getters/setters being marked as deprecated in 8.0.0 in our API docs
  • Loading branch information
sgtcoolguy committed Jul 27, 2018
1 parent 642a019 commit 39c330f
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 9 deletions.
Expand Up @@ -62,7 +62,8 @@
<#local signature>
<@getMethodSignature method.args, method.returnType, false/>
</#local>
<#nested methodName_index==0, methodName, method, signature, methodName_has_next>
<#local isDynamic = isDynamicProperty(methodName)>
<#nested methodName_index==0, methodName, method, signature, isDynamic, methodName_has_next>
</#list>
</#if>
</#macro>
Expand Down
Expand Up @@ -113,7 +113,7 @@ Local<FunctionTemplate> ${className}::getProxyTemplate(Isolate* isolate)
FunctionTemplate::New(isolate, titanium::Proxy::inherit<${className}>));

// Method bindings --------------------------------------------------------
<@Proxy.listMethods ; isFirst, name, method, signature>
<@Proxy.listMethods ; isFirst, name, method>
titanium::SetProtoMethod(isolate, t, "${method.apiName}", ${className}::${method.apiName});
</@Proxy.listMethods>

Expand Down Expand Up @@ -233,7 +233,7 @@ Local<FunctionTemplate> ${className}::getProxyTemplate(Isolate* isolate)
}

// Methods --------------------------------------------------------------------
<@Proxy.listMethods ; isFirst, name, method, signature>
<@Proxy.listMethods ; isFirst, name, method, signature, isDynamic>
void ${className}::${method.apiName}(const FunctionCallbackInfo<Value>& args)
{
LOGD(TAG, "${method.apiName}()");
Expand All @@ -256,6 +256,14 @@ void ${className}::${method.apiName}(const FunctionCallbackInfo<Value>& args)
jvalue* jArguments = 0;
</#if>

<#if method.args?size == 0 && name[0..2] == "get" && isDynamic>
<#assign propertyName = name[3]?lower_case + name[4..]>
LOGW(TAG, "Automatic getter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please access the property in standard JS style: obj.${propertyName}; or obj['${propertyName}'];");
<#elseif method.args?size == 1 && name[0..2] == "set" && isDynamic>
<#assign propertyName = name[3]?lower_case + name[4..]>
LOGW(TAG, "Automatic setter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please modify the property in standard JS style: obj.${propertyName} = value; or obj['${propertyName}'] = value;");
</#if>

jobject javaProxy = proxy->getJavaObject();
if (javaProxy == NULL) {
args.GetReturnValue().Set(v8::Undefined(isolate));
Expand Down
12 changes: 12 additions & 0 deletions android/runtime/v8/src/native/Proxy.cpp
Expand Up @@ -96,12 +96,14 @@ static Local<Value> getPropertyForProxy(Isolate* isolate, Local<Name> property,
return Undefined(isolate);
}

// This variant is used when accessing a property in standard JS fashion (i.e. obj.text or obj['text'])
void Proxy::getProperty(Local<Name> property, const PropertyCallbackInfo<Value>& args)
{
Isolate* isolate = args.GetIsolate();
args.GetReturnValue().Set(getPropertyForProxy(isolate, property->ToString(isolate), args.Holder()));
}

// This variant is used when accessing a property through a getter method (i.e. obj.getText())
void Proxy::getProperty(const FunctionCallbackInfo<Value>& args)
{
Isolate* isolate = args.GetIsolate();
Expand All @@ -117,6 +119,10 @@ void Proxy::getProperty(const FunctionCallbackInfo<Value>& args)
return;
}

// Spit out deprecation notice to use normal property getter
v8::String::Utf8Value propertyKey(name);
LOGW(TAG, "Automatic getter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please access the property in standard JS style: obj.%s; or obj['%s'];", *propertyKey, *propertyKey);

args.GetReturnValue().Set(getPropertyForProxy(isolate, name, args.Holder()));
}

Expand Down Expand Up @@ -177,12 +183,14 @@ static void onPropertyChangedForProxy(Isolate* isolate, Local<String> property,
setPropertyOnProxy(isolate, property, value, proxyObject);
}

// This variant is used when accessing a property in a standard way and setting a value (i.e. obj.text = 'whatever')
void Proxy::onPropertyChanged(Local<Name> property, Local<Value> value, const v8::PropertyCallbackInfo<void>& info)
{
Isolate* isolate = info.GetIsolate();
onPropertyChangedForProxy(isolate, property->ToString(isolate), value, info.Holder());
}

// This variant is used when accessing a property through a getter method (i.e. setText('whatever'))
void Proxy::onPropertyChanged(const v8::FunctionCallbackInfo<v8::Value>& args)
{
Isolate* isolate = args.GetIsolate();
Expand All @@ -192,6 +200,10 @@ void Proxy::onPropertyChanged(const v8::FunctionCallbackInfo<v8::Value>& args)
}

Local<String> name = args.Data()->ToString(isolate);
// Spit out deprecation notice to use normal property setter, not setX() style method.
v8::String::Utf8Value propertyKey(name);
LOGW(TAG, "Automatic setter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please modify the property in standard JS style: obj.%s = value; or obj['%s'] = value;", *propertyKey, *propertyKey);

Local<Value> value = args[0];
onPropertyChangedForProxy(isolate, name, value, args.Holder());
}
Expand Down
Expand Up @@ -233,11 +233,8 @@ public boolean getHidden()
return tbf.isHidden();
}

// clang-format off
@Kroll.method
@Kroll.getProperty
public String[] getDirectoryListing()
// clang-format on
{
if (!isDirectory()) {
return null;
Expand Down
4 changes: 2 additions & 2 deletions apidoc/docgen.js
Expand Up @@ -327,7 +327,7 @@ function generateAccessors(apis, className) {
rv.push({
name: 'get' + api.name.charAt(0).toUpperCase() + api.name.slice(1),
summary: 'Gets the value of the <' + className + '.' + api.name + '> property.',
deprecated: api.deprecated || null,
deprecated: api.deprecated || { since: '8.0.0', notes: 'Access <' + className + '.' + api.name + '> instead.' },
platforms: api.platforms,
since: api.since,
returns: { type: api.type, __subtype: 'return' },
Expand All @@ -343,7 +343,7 @@ function generateAccessors(apis, className) {
rv.push({
name: 'set' + api.name.charAt(0).toUpperCase() + api.name.slice(1),
summary: 'Sets the value of the <' + className + '.' + api.name + '> property.',
deprecated: api.deprecated || null,
deprecated: api.deprecated || { since: '8.0.0', notes: 'Set the value using <' + className + '.' + api.name + '> instead.' },
platforms: api.platforms,
since: api.since,
parameters: [ {
Expand Down
10 changes: 10 additions & 0 deletions iphone/Classes/KrollMethod.m
Expand Up @@ -231,13 +231,23 @@ - (id)call:(NSArray *)args
{
// special property setter delegator against the target
if (type == KrollMethodPropertySetter && [args count] == 1) {
// Spit out a deprecation warning to use normal property setter!
// This is the code path followed for delegating access for soemthing like
// Ti.UI.Label#setText(). Which delegates through TiProxy.m TiProxyDelegate code
// Other code path is handled in KrollObject.m
DebugLog(@"[WARN] Automatic setter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please modify the property in standard JS style: obj.%@ = value; or obj['%@'] = value;", name, name);
id newValue = [KrollObject nonNull:[args objectAtIndex:0]];
[self updateJSObjectWithValue:newValue forKey:name];
[target setValue:newValue forKey:name];
return self;
}
// special property getter delegator against the target
if (type == KrollMethodPropertyGetter) {
// Spit out a deprecation warning to use normal property accessor!
// This is the code path followed for delegating access for something like
// Ti.UI.Label#getText(). Which delegates through TiProxy.m TiProxyDelegate code
// Other code path is handled in KrollObject.m
DebugLog(@"[WARN] Automatic getter methods for properties are in SDK 8.0.0 and will be removed in SDK 9.0.0. Please access the property in standard JS style: obj.%@ or obj['%@']", name, name);
// hold, see below
id result = [target valueForKey:name];
[self updateJSObjectWithValue:result forKey:name];
Expand Down
11 changes: 10 additions & 1 deletion iphone/Classes/KrollObject.m
Expand Up @@ -590,7 +590,7 @@ - (id)_valueForKey:(NSString *)key
[[NSCharacterSet uppercaseLetterCharacterSet] characterIsMember:[key characterAtIndex:3]]) {
// This is PROBABLY a request for an internal setter (either setX('a') or setX('a','b')). But
// it could also be:
// * Pulling a user-defined property prefixed with 'get'
// * Pulling a user-defined property prefixed with 'set'
// * Autogenerating a getter/setter
// In the event of the former, we actually have to actually pull a jump to
// returning the property's appropriate type, as below in the general case.
Expand All @@ -615,6 +615,12 @@ - (id)_valueForKey:(NSString *)key
} else {
selector = NSSelectorFromString([key stringByAppendingString:@":"]);
if ([target respondsToSelector:selector]) {
// Assume that if there's a getter with same property name, that this is just exposing the property's setter method to JS when it shouldn't be
if ([target respondsToSelector:NSSelectorFromString(propertyKey)]) {
// This is the code path for delegating something like Ti.Filesystem.File#setHidden(), see KrollMethod for other cases (when type is KrollMethodPropertySetter, the last option in this if block below)
// Spit out a deprecation warning to use normal property setter!
DebugLog(@"[WARN] Automatic setter methods for properties are deprecated in SDK 8.0.0 and will be removed in SDK 9.0.0. Please modify the property in standard JS style: obj.%@ = value; or obj['%@'] = value;", propertyKey, propertyKey);
}
[result setSelector:selector];
} else {
// Either a custom property, OR a request for an autogenerated setter
Expand Down Expand Up @@ -654,6 +660,9 @@ - (id)_valueForKey:(NSString *)key
NSString *partkey = [self propercase:key index:3];
SEL selector = NSSelectorFromString(partkey);
if ([target respondsToSelector:selector]) {
// Spit out a deprecation warning to use normal property accessor!
// This is the code path for delegating something like Ti.Filesystem.File#getHidden(), see KrollMethod for other cases (when type is KrollMethodPropertyGetter, the last option in this if block below)
DebugLog(@"[WARN] Automatic getter methods for properties are in SDK 8.0.0 and will be removed in SDK 9.0.0. Please access the property in standard JS style: obj.%@ or obj['%@']", partkey, partkey);
[result setSelector:selector];
[result setType:KrollMethodGetter];
return [result autorelease];
Expand Down

0 comments on commit 39c330f

Please sign in to comment.