fix(ios): avoid leaking objects in hasProperty callback #10524
Conversation
Generated by 🚫 dangerJS |
|
@sgtcoolguy added tests to verify the property checks. This revealed some differences between Android and iOS we might want to fix for parity. |
| describe('Submodules', () => { | ||
| it('should check for sub-module', () => { | ||
| Ti.should.have.property('UI'); | ||
| // @fixme this check throws an error on android |
There was a problem hiding this comment.
The way to mark this in the test is to do it.androidBroken('should check for sub-module', () => {
| }); | ||
|
|
||
| describe('Custom properties', () => { | ||
| it.ios('should check for custom properties', () => { |
There was a problem hiding this comment.
why limit this to iOS only?
There was a problem hiding this comment.
Android throws an error when doing .not.have.property checks directly on modules. I'll mark it as it.androidBroken together with the other one above.
|
|
||
| it.ios('should check for dynamic getter method', () => { | ||
| const view = Ti.UI.createView(); | ||
| view.should.have.property('getFoo'); |
There was a problem hiding this comment.
why should this pass? It shouldn't respond true here as there is no "foo" property.
There was a problem hiding this comment.
That's iOS getter/setter magic. It first tries to find an actually existing property on the proxy and tries to set that. If not found, it will fallback to set it as a "dynamic property" on the proxy. So everything starting with set or get is automatically available on iOS.
This results in these statements being equivalent on iOS:
view.foo = 'bar';
console.log(view.foo);view.setFoo('bar');
console.log(view.getFoo());|
|
||
| it.ios('should check for dynamic setter method', () => { | ||
| const view = Ti.UI.createView(); | ||
| view.should.have.property('setFoo'); |
There was a problem hiding this comment.
again, why does this pass? This looks like a bug to me, there's no foo property.
|
@sgtcoolguy marked the two tests as broken on android. For the weird/magical iOS behavior see my other comment. |
JIRA: https://jira.appcelerator.org/browse/TIMOB-26452
Optional Description:
This fixes two issues at once:
hasPropertycallback in [TIMOB-26179] Check for property existence using hasProperty #10150 and hangingJSValueRefs that were not added to the JS object graph after retrieval. This reworks the whole implementation by checking for all the magic that is happening in- (id)valueForKey:(NSString *)keyand just returning true or false without accessing the actual value of the property.@sgtcoolguy This introduces some duplicate code since the checks are now done in
hasPropertyand invalueForKey. I have some ideas how to resolve this by isolating the checks and match them with an enum. We would then just check for the enum's value and perform the appropriate actions in both methods. However, i didn't want to refactor too much of the existing code in such a critical point of our core. We might want to pair this with setter and getter removal in 9.0.0 when we need to touchvalueForKeyanyway.