Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TIMOB-27082] fix(ios): restore SDK < 8.1.0 compatibility #10897

Merged
merged 13 commits into from
May 31, 2019

Conversation

hansemannn
Copy link
Collaborator

@hansemannn hansemannn commented May 15, 2019

JIRA: https://jira.appcelerator.org/browse/TIMOB-27082

Note: There may be more classes that are affected by moving to the JSCore ObjC API, so this may need to be extended further. We're currently testing all cases.

cc @sgtcoolguy

EDIT: No more affected cases for us.

@build build added this to the 8.1.0 milestone May 15, 2019
@build build requested a review from a team May 15, 2019 22:06
@build
Copy link
Contributor

build commented May 15, 2019

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

Messages
📖

💾 Here's the generated SDK zipfile.

📖 🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖

✅ All tests are passing
Nice one! All 3695 tests are passing.
(There are 466 tests skipped)

Generated by 🚫 dangerJS against 9aea25e

@janvennemann
Copy link
Contributor

@sgtcoolguy i did a quick check of #10381 and it looks like a few more proxies are affected:

  • TiCalendarAlert
  • TiCalendarAttendee
  • TiCalendarCalendar
  • TiCalendarEvent
  • TiCalendarRecurrenceRule

@sgtcoolguy
Copy link
Contributor

@hansemannn Technically, anything that went from TiProxy -> ObjcProxy hierarchy would be affected in this way, but obviously we don't expect people to instantiate any modules as they're intended to be singletons.

So I guess that'd mean these particular types are affected:

  • TiBlob
  • TiDatabaseProxy
  • TiDatabaseResultSetProxy
  • TiCalendarAlert
  • TiCalendarAttendee
  • TiCalendarCalendar
  • TiCalendarEvent
  • TiCalendarRecurrenceRule

And just adding back the old init methods seems to fix the issue for you?

@hansemannn
Copy link
Collaborator Author

hansemannn commented May 17, 2019

Yep, that works, because the linked (compiled) modules only search for the selectors and performs them if found. For classic proxies, having the default _initWithPageContext: constructor in TiProxy will fix most use-cases. In TiBlob, having custom constructors that accepted more than the context caused the issues and this PR resolves those.

EDIT: We should also keep in mind that those proxies have usually been autorelease'd and the new ObjcProxy doesn't, so maybe we can check on that topic as well.

@sgtcoolguy
Copy link
Contributor

@hansemannn I've added back the old init page context methods for the other proxies I listed above. Can you confirm this works fine for you, and then I'll merge it?

@@ -111,6 +111,26 @@ - (NSUInteger)size
}
GETTER_IMPL(NSUInteger, size, Size);

- (id)_initWithPageContext:(id<TiEvaluator>)pageContext
{
return [[TiBlob alloc] init];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default _initWithPageContext: constructor should be re-added to the parent class (TiProxy) again, since other proxies (especially in modules) may be constructed that way as well.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I can do that too, I suppose.

Related: why do we need to alloc calls here? Typical usage before was:

[[TiBlob alloc] _initWithPageContext:context];

so should the redirecting version just be:

- (id)_initWithPageContext:(id<TiEvaluator>)pageContext
{
  return [self init];
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, no alloc required!

@@ -15,6 +15,11 @@ @implementation TiCalendarRecurrenceRule

#pragma mark - Internals

- (id)_initWithPageContext:(id<TiEvaluator>)context rule:(EKRecurrenceRule *)rule_
{
return [self initWithRule:rule];
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sgtcoolguy rule_

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can run the project locally in iphone/iphone/Titanium.xcodeproj.

@sgtcoolguy sgtcoolguy merged commit 2aeb8d0 into tidev:master May 31, 2019
hansemannn added a commit to hansemannn/titanium_mobile that referenced this pull request Jul 8, 2019
add _initWithPageContext to base ObjcProxy, add other legacy initializer methods to changed proxies

Fixes TIMOB-27082
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants